diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 376ba56d8dd9..18ec19a12923 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -366,6 +366,7 @@ def test_switch_to_unknown_source_library_preserves_settings(self): self.assertEqual(resp.status_code, 200) lc_block = modulestore().get_item(lc_block.location) + # Source library id should be set to the new bad one... assert lc_block.source_library_id == bad_library_id # ...but old source library version should be preserved... diff --git a/cms/djangoapps/contentstore/toggles.py b/cms/djangoapps/contentstore/toggles.py index ac8469678f78..a71c1c8788bd 100644 --- a/cms/djangoapps/contentstore/toggles.py +++ b/cms/djangoapps/contentstore/toggles.py @@ -178,6 +178,25 @@ def use_add_game_block(): return ENABLE_ADD_GAME_BLOCK_FLAG.is_enabled() +# .. toggle_name: new_core_editors.use_new_library_content_editor +# .. toggle_implementation: WaffleFlag +# .. toggle_default: False +# .. toggle_description: This flag enables the use of the new library xblock editor +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2023-10-30 +# .. toggle_target_removal_date: 2025-1-30 +# .. toggle_tickets: https://github.com/openedx/edx-platform/issues/33640 +# .. toggle_warning: +ENABLE_NEW_LIBRARY_CONTENT_EDITOR_FLAG = WaffleFlag('new_core_editors.use_new_library_content_editor', __name__) + + +def use_new_library_content_editor(): + """ + Returns a boolean if new library content block editor is enabled + """ + return ENABLE_NEW_LIBRARY_CONTENT_EDITOR_FLAG.is_enabled() + + # .. toggle_name: contentstore.individualize_anonymous_user_id # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 91e85f8df660..05921af92ec3 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1892,6 +1892,7 @@ def get_container_handler_context(request, usage_key, course, xblock): # pylint 'unit': unit, 'is_unit_page': is_unit_page, 'is_collapsible': is_library_xblock, + 'is_library_xblock': is_library_xblock, 'subsection': subsection, 'section': section, 'position': index, diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 8b122d8c8da0..f23712be0c25 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -178,6 +178,16 @@ def handle_xblock(request, usage_key_string=None): xblock, is_concise=True ) return JsonResponse(ancestor_info) + elif "childrenInfo" in fields: + xblock = get_xblock(usage_key, request.user) + children_info = _create_xblock_child_info( + xblock, + course_outline=None, + graders=None, + include_children_predicate=ALWAYS, + is_concise=True + ) + return JsonResponse(children_info) # TODO: pass fields to get_block_info and only return those with modulestore().bulk_operations(usage_key.course_key): response = get_block_info(get_xblock(usage_key, request.user)) @@ -825,7 +835,7 @@ def get_block_info( rewrite_static_links=True, include_ancestor_info=False, include_publishing_info=False, - include_children_predicate=False, + include_children_predicate=NEVER, ): """ metadata, data, id representation of a leaf block fetcher. diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 3268b60e416a..1b9e91e90e2b 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -121,6 +121,11 @@ function($, _, Backbone, gettext, BasePage, this.unitOutlineView.render(); } + if (this.isLibraryContentPage) { + this.selectedLibraryComponents = []; + this.storedSelectedLibraryComponents = []; + this.getSelectedLibraryComponents(); + } this.listenTo(Backbone, 'move:onXBlockMoved', this.onXBlockMoved); }, @@ -370,11 +375,13 @@ function($, _, Backbone, gettext, BasePage, var useNewTextEditor = primaryHeader.attr('use-new-editor-text'), useNewVideoEditor = primaryHeader.attr('use-new-editor-video'), useNewProblemEditor = primaryHeader.attr('use-new-editor-problem'), + useNewLibraryContentEditor = primaryHeader.attr('use-new-editor-library-content'), blockType = primaryHeader.attr('data-block-type'); if((useNewTextEditor === 'True' && blockType === 'html') || (useNewVideoEditor === 'True' && blockType === 'video') || (useNewProblemEditor === 'True' && blockType === 'problem') + || (useNewLibraryContentEditor === 'True' && blockType === 'library_content') ) { var destinationUrl = primaryHeader.attr('authoring_MFE_base_url') + '/' + blockType + '/' + encodeURI(primaryHeader.attr('data-usage-id')); window.location.href = destinationUrl; @@ -639,12 +646,11 @@ function($, _, Backbone, gettext, BasePage, 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); + self.selectedLibraryComponents = Array.from(data.candidates); + self.storedSelectedLibraryComponents = Array.from(data.candidates); } ); }, @@ -655,7 +661,7 @@ function($, _, Backbone, gettext, BasePage, e.preventDefault(); $.postJSON( ModuleUtils.getUpdateUrl(locator) + '/handler/submit_studio_edits', - {values: {source_block_ids: self.storedSelectedLibraryComponents}}, + {values: {candidates: self.storedSelectedLibraryComponents}}, function() { self.selectedLibraryComponents = Array.from(self.storedSelectedLibraryComponents); self.toggleSaveButton(); @@ -665,6 +671,7 @@ function($, _, Backbone, gettext, BasePage, 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); diff --git a/cms/templates/container.html b/cms/templates/container.html index d61ce60e9189..2f0728e11210 100644 --- a/cms/templates/container.html +++ b/cms/templates/container.html @@ -176,7 +176,15 @@

${_("Page Actions")}

${_("Preview")} + % else: + % if is_library_xblock: +
  • + +
    • + % endif
  • diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 9ae3a3a5dd21..2bc1219ddace 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -7,12 +7,20 @@ from openedx.core.djangolib.js_utils import ( dump_js_escaped_json, js_escaped_string ) -from cms.djangoapps.contentstore.toggles import use_new_text_editor, use_new_problem_editor, use_new_video_editor, use_video_gallery_flow, use_tagging_taxonomy_list_page +from cms.djangoapps.contentstore.toggles import( + use_new_text_editor, + use_new_problem_editor, + use_new_video_editor, + use_new_library_content_editor, + use_video_gallery_flow, + use_tagging_taxonomy_list_page, +) %> <% use_new_editor_text = use_new_text_editor() use_new_editor_video = use_new_video_editor() use_new_editor_problem = use_new_problem_editor() +use_new_editor_library_content = use_new_library_content_editor() use_new_video_gallery_flow = use_video_gallery_flow() use_tagging = use_tagging_taxonomy_list_page() xblock_url = xblock_studio_url(xblock) @@ -80,6 +88,7 @@ use-new-editor-text = ${use_new_editor_text} use-new-editor-video = ${use_new_editor_video} use-new-editor-problem = ${use_new_editor_problem} + use-new-editor-library-content = ${use_new_editor_library_content} use-video-gallery-flow = ${use_new_video_gallery_flow} authoring_MFE_base_url = ${get_editor_page_base_url(xblock.location.course_key)} data-block-type = ${xblock.scope_ids.block_type} diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 616cf68f4b62..5276026e1303 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -1,21 +1,30 @@ """ Content Library Transformer. """ - +from __future__ import annotations import json import logging +from functools import partial +from typing import Any, Callable + from eventtracking import tracker +from opaque_keys.edx.keys import UsageKey from common.djangoapps.track import contexts from lms.djangoapps.courseware.models import StudentModule +from openedx.core.djangoapps.content.block_structure.block_structure import ( + BlockStructureModulestoreData, + BlockStructureBlockData, +) from openedx.core.djangoapps.content.block_structure.transformer import ( BlockStructureTransformer, FilteringTransformerMixin ) from xmodule.library_content_block import LibraryContentBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.util.keys import BlockKey from ..utils import get_student_module_as_dict @@ -34,7 +43,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo READ_VERSION = 1 @classmethod - def name(cls): + def name(cls) -> str: """ Unique identifier for the transformer's class; same identifier used in setup.py. @@ -42,18 +51,22 @@ def name(cls): return "library_content" @classmethod - def collect(cls, block_structure): + def collect(cls, block_structure: BlockStructureModulestoreData) -> None: """ Collects any information that's necessary to execute this transformer's transform method. """ block_structure.request_xblock_fields('mode') + block_structure.request_xblock_fields('shuffle') block_structure.request_xblock_fields('max_count') + block_structure.request_xblock_fields('manual') + block_structure.request_xblock_fields('candidates') + block_structure.request_xblock_fields('source_library_id') block_structure.request_xblock_fields('category') store = modulestore() # needed for analytics purposes - def summarize_block(usage_key): + def summarize_block(usage_key: UsageKey) -> dict[str, str | None]: """ Basic information about the given block """ orig_key, orig_version = store.get_block_original_usage(usage_key) return { @@ -73,18 +86,35 @@ def summarize_block(usage_key): summary = summarize_block(child_key) block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary) - def transform_block_filters(self, usage_info, block_structure): - all_library_children = set() - all_selected_children = set() + def transform_block_filters( + self, + usage_info: Any, + block_structure: BlockStructureBlockData, + ) -> list[Callable[[UsageKey], bool]]: + """ + Returns a list of functions which filter blocks out of this user's structure. + + For this particular Transformer, we only need one filter: + "given a block, remove it IFF it's under a library_content block BUT it is not selected for this user" + """ + all_library_children: set[UsageKey] = set() + all_selected_children: set[UsageKey] = set() for block_key in block_structure: if block_key.block_type != 'library_content': continue library_children = block_structure.get_children(block_key) if library_children: all_library_children.update(library_children) - selected = [] - mode = block_structure.get_xblock_field(block_key, 'mode') - max_count = block_structure.get_xblock_field(block_key, 'max_count') + selected: list[BlockKey] = [] + shuffle: bool = block_structure.get_xblock_field(block_key, 'shuffle') + manual: bool = block_structure.get_xblock_field(block_key, 'manual') + max_count: int = block_structure.get_xblock_field(block_key, 'max_count') + candidates: list[UsageKey] = [ + UsageKey.from_string(key_string) + for key_string in block_structure.get_xblock_field(block_key, 'candidates') + ] + source_library_id: str = block_structure.get_xblock_field(block_key, 'source_library_id') + if max_count < 0: max_count = len(library_children) @@ -94,13 +124,21 @@ def transform_block_filters(self, usage_info, block_structure): # Add all selected entries for this user for this # library block to the selected list. block_type, block_id = selected_block - usage_key = usage_info.course_key.make_usage_key(block_type, block_id) + usage_key = usage_info.course_key.make_usage_key(*selected_block) if usage_key in library_children: - selected.append(selected_block) + selected.append(BlockKey(*selected_block)) # Update selected previous_count = len(selected) - block_keys = LibraryContentBlock.make_selection(selected, library_children, max_count, mode) + block_keys = LibraryContentBlock.make_selection( + usage_key=block_key, + old_selected=selected, + all_children=library_children, + candidates=candidates, + max_count=max_count, + manual=manual, + shuffle=shuffle, + ) selected = block_keys['selected'] # Save back any changes @@ -124,9 +162,9 @@ def transform_block_filters(self, usage_info, block_structure): block_keys, usage_info.user.id, ) - all_selected_children.update(usage_info.course_key.make_usage_key(s[0], s[1]) for s in selected) + all_selected_children.update(usage_info.course_key.make_usage_key(*s) for s in selected) - def check_child_removal(block_key): + def check_child_removal(block_key: UsageKey): """ Return True if selected block should be removed. @@ -141,12 +179,20 @@ def check_child_removal(block_key): return [block_structure.create_removal_filter(check_child_removal)] - def _publish_events(self, block_structure, location, previous_count, max_count, block_keys, user_id): + def _publish_events( + self, + block_structure: BlockStructureBlockData, + location: UsageKey, + previous_count: int, + max_count: int, + block_keys: dict[str, list[BlockKey]], + user_id, + ) -> None: """ Helper method to publish events for analytics purposes """ - def format_block_keys(keys): + def format_block_keys(keys: list[BlockKey]) -> list: """ Helper function to format block keys """ @@ -158,7 +204,7 @@ def format_block_keys(keys): json_result.append(info) return json_result - def publish_event(event_name, result, **kwargs): + def publish_event(event_name: str, result: list, **kwargs) -> None: """ Helper function to publish an event for analytics purposes """ @@ -197,7 +243,7 @@ class ContentLibraryOrderTransformer(BlockStructureTransformer): READ_VERSION = 1 @classmethod - def name(cls): + def name(cls) -> str: """ Unique identifier for the transformer's class; same identifier used in setup.py @@ -205,15 +251,14 @@ def name(cls): return "library_content_randomize" @classmethod - def collect(cls, block_structure): + def collect(cls, block_structure: BlockStructureModulestoreData) -> None: """ Collects any information that's necessary to execute this transformer's transform method. """ # There is nothing to collect - pass # pylint:disable=unnecessary-pass - def transform(self, usage_info, block_structure): + def transform(self, usage_info: Any, block_structure: BlockStructureBlockData) -> None: """ Transforms the order of the children of the randomized content block to match the order of the selections made and stored in the XBlock 'selected' field. @@ -222,12 +267,16 @@ def transform(self, usage_info, block_structure): if block_key.block_type != 'library_content': continue - library_children = block_structure.get_children(block_key) + library_children: list[UsageKey] = block_structure.get_children(block_key) if library_children: state_dict = get_student_module_as_dict(usage_info.user, usage_info.course_key, block_key) - current_children_blocks = {block.block_id for block in library_children} - current_selected_blocks = {item[1] for item in state_dict.get('selected', [])} + current_children_blocks: set[str] = { + block.block_id for block in library_children + } + current_selected_blocks: set[str] = { + block_id for _block_type, block_id in state_dict.get('selected', []) + } # As the selections should have already been made by the ContentLibraryTransformer, # the current children of the library_content block should be the same as the stored @@ -242,5 +291,13 @@ def transform(self, usage_info, block_structure): usage_info.user.username ) else: - ordering_data = {block[1]: position for position, block in enumerate(state_dict['selected'])} - library_children.sort(key=lambda block, data=ordering_data: data[block.block_id]) + ordering_data: dict[str, int] = { + block_id: position + for position, (_block_type, block_id) + in enumerate(state_dict['selected']) + } + get_position_for_child: Callable[[UsageKey], int] = partial( + lambda child_usage_key, ordering: ordering[child_usage_key.block_id], + ordering_data, # Avoid directly using ordering_data in lambda, since it's a loop variable. + ) + library_children.sort(key=get_position_for_child) diff --git a/mypy.ini b/mypy.ini index 5027c3bb5595..099890a7ed8a 100644 --- a/mypy.ini +++ b/mypy.ini @@ -12,6 +12,8 @@ files = openedx/core/djangoapps/xblock, openedx/core/types, openedx/core/djangoapps/content_tagging, + lms/djangoapps/course_blocks/transformers/library_content.py, + xmodule/library_content_block.py, xmodule/util/keys.py [mypy.plugins.django-stubs] diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 26e7c652d983..e7c7ceae88a0 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -131,6 +131,7 @@ from openedx.core.djangolib import blockstore_cache from openedx.core.djangolib.blockstore_cache import BundleCache from xmodule.library_root_xblock import LibraryRoot as LibraryRootV1 + from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 82a2c48ed8df..fc742b5a6ce1 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -317,6 +317,7 @@ def _sync_children( source_blocks = [] library_key = dest_block.source_library_key filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE) + library = library_api.get_v1_or_v2_library(library_key, version=library_version) if not library: task.status.fail(f"Requested library {library_key} not found.") diff --git a/requirements/edx/development.in b/requirements/edx/development.in index 00c9e533b126..5ba43fdc9363 100644 --- a/requirements/edx/development.in +++ b/requirements/edx/development.in @@ -21,5 +21,6 @@ django-stubs # Typing stubs for Django, so it works w djangorestframework-stubs # Typing stubs for DRF mypy # static type checking pywatchman # More efficient checking for runserver reload trigger events +types-bleach # Typing stubs for bleach, so it works with mypy vulture # Detects possible dead/unused code, used in scripts/find-dead-code.sh watchdog # Used by `npm run watch` to auto-recompile when assets are changed diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index bfe3fbd2b77e..616e17d81781 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -2039,7 +2039,9 @@ tqdm==4.66.1 # -r requirements/edx/testing.txt # nltk # openai -types-pytz==2024.1.0.20240203 +types-bleach==6.1.0.1 + # via -r requirements/edx/development.in +types-pytz==2023.3.1.1 # via django-stubs types-pyyaml==6.0.12.12 # via diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index e4c4af267a1f..880e5d8b365c 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -8,6 +8,7 @@ import random from copy import copy from gettext import ngettext, gettext +from typing import TYPE_CHECKING, Any, Callable import bleach from django.conf import settings @@ -17,17 +18,20 @@ from lxml.etree import XMLSyntaxError from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from opaque_keys.edx.keys import UsageKey from rest_framework import status from web_fragments.fragment import Fragment from webob import Response from xblock.completable import XBlockCompletionMode from xblock.core import XBlock from xblock.fields import Boolean, Integer, List, Scope, String +from xblock.utils.studio_editable import StudioEditableXBlockMixin from xmodule.capa.responsetypes import registry 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.util.keys import BlockKey, derive_key from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.xml_block import XmlMixin from xmodule.x_module import ( @@ -38,6 +42,13 @@ shim_xmodule_js, ) + +if TYPE_CHECKING: + # This class is only needed for a type annotation. + # To avoid circular import, only import it for type checking. + from xmodule.library_tools import LibraryToolsService + + # 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 @@ -47,23 +58,28 @@ ANY_CAPA_TYPE_VALUE = 'any' -def _get_human_name(problem_class): +def _get_human_name(problem_class: type) -> str: """ Get the human-friendly name for a problem type. """ return getattr(problem_class, 'human_name', problem_class.__name__) -def _get_capa_types(): +def _get_capa_types() -> list[dict[str, str]]: """ Gets capa types tags and labels """ capa_types = {tag: _get_human_name(registry.get_class_for_tag(tag)) for tag in registry.registered_tags()} - - return [{'value': ANY_CAPA_TYPE_VALUE, 'display_name': _('Any Type')}] + sorted([ - {'value': capa_type, 'display_name': caption} - for capa_type, caption in capa_types.items() - ], key=lambda item: item.get('display_name')) + return [ + {'value': ANY_CAPA_TYPE_VALUE, 'display_name': _('Any Type')}, + *sorted( + [ + {'value': capa_type, 'display_name': caption} + for capa_type, caption in capa_types.items() + ], + key=lambda item: item['display_name'] + ), + ] class LibraryToolsUnavailable(ValueError): @@ -79,8 +95,9 @@ def __init__(self): @XBlock.wants('user') @XBlock.needs('mako') class LibraryContentBlock( - MakoTemplateBlockBase, XmlMixin, + MakoTemplateBlockBase, + StudioEditableXBlockMixin, XModuleToXBlockMixin, ResourceTemplates, XModuleMixin, @@ -95,6 +112,8 @@ class LibraryContentBlock( any particular student. """ # pylint: disable=abstract-method + + editable_fields = ("candidates",) has_children = True has_author_view = True @@ -105,68 +124,87 @@ class LibraryContentBlock( show_in_read_only_mode = True - # noinspection PyMethodParameters @classproperty - def completion_mode(cls): # pylint: disable=no-self-argument + def completion_mode(cls) -> str: # pylint: disable=no-self-argument """ Allow overriding the completion mode with a feature flag. - This is a property, so it can be dynamically overridden in tests, as it is not evaluated at runtime. """ - if settings.FEATURES.get('MARK_LIBRARY_CONTENT_BLOCK_COMPLETE_ON_VIEW', False): + if settings.FEATURES.get('MARK_LIBRARY_CONTENT_BLOCK_COMPLETE_ON_VIEW', False): # type: ignore return XBlockCompletionMode.COMPLETABLE return XBlockCompletionMode.AGGREGATOR - display_name = String( + resources_dir = 'assets/library_content' + + # NOTE: These field type annotations are correct on an *instance* of LibraryContentBlock, but on the + # LibraryContentBlock class itself, these would all actually be XBlock Field objects. + # Until then, these annotations are the only way to thoroughly typecheck this module. + display_name: str = String( display_name=_("Display Name"), help=_("The display name for this component."), - default="Randomized Content Block", + default="Library Content", scope=Scope.settings, ) - source_library_id = String( + source_library_id: str | None = String( display_name=_("Library"), help=_("Select the library from which you want to draw content."), scope=Scope.settings, values_provider=lambda instance: instance.source_library_values(), ) - source_library_version = String( + source_library_version: str | None = String( # This is a hidden field that stores the version of source_library when we last pulled content from it display_name=_("Library Version"), scope=Scope.settings, ) - mode = String( - display_name=_("Mode"), - help=_("Determines how content is drawn from the library"), - default="random", - values=[ - {"display_name": _("Choose n at random"), "value": "random"} - # Future addition: Choose a new random set of n every time the student refreshes the block, for self tests - # Future addition: manually selected blocks - ], - scope=Scope.settings, - ) - max_count = Integer( + max_count: int = 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( + capa_type: str = String( display_name=_("Problem Type"), help=_('Choose a problem type to fetch from the library. If "Any Type" is selected no filtering is applied.'), default=ANY_CAPA_TYPE_VALUE, values=_get_capa_types(), scope=Scope.settings, ) - selected = List( - # This is a list of (block_type, block_id) tuples used to record - # which random/first set of matching blocks was selected per user + candidates: list[str] = List( + # This is a list of stringified library block usage keys representing the library subset that the author + # has manually picked as candidates for selection. Note: these are the keys of *blocks in the + # source library*, not of the keys of this block's children. + display_name=_("Manually Selected Blocks"), + default=[], + scope=Scope.settings, + ) + selected: list[list[str]] = List( + # This is a list of [block_type, block_id] pairs used to record + # which set of matching blocks was selected per user default=[], scope=Scope.user_state, ) + shuffle: bool = Boolean( + # Do we shuffle (randomize order) of selected blocks for each learner? + # True -> Order is randomized for each learner. \n False -> Original order from candidates/children is used. + # False -> is the block content only drawn from content which is in the candidates list? + display_name=_("Shuffle Components"), + help=_("When enabled, each learner will see the components in a randomized order."), + default=True, + scope=Scope.settings, + + ) + manual: bool = Boolean( + # Should selected blocks be limited to the manually-picked candidates? + # True -> Draw selections from `candidates`. + # False -> Draw selections from `children`. + display_name=_("Limit Components to Selection"), + help=_("When enabled, only the checked-off components on the 'View' page are available to learners."), + default=False, + scope=Scope.settings, + ) # This cannot be called `show_reset_button`, because children blocks inherit this as a default value. - allow_resetting_children = Boolean( + allow_resetting_children: bool = Boolean( display_name=_("Show Reset Button"), help=_("Determines whether a 'Reset Problems' button is shown, so users may reset their answers and reshuffle " "selected items."), @@ -181,74 +219,30 @@ def source_library_key(self): Supports either library v1 or library v2 locators. """ - try: - return LibraryLocator.from_string(self.source_library_id) - except InvalidKeyError: - return LibraryLocatorV2.from_string(self.source_library_id) + return self.get_source_library_key(self.source_library_id) @classmethod - def make_selection(cls, selected, children, max_count, mode): + def get_source_library_key(cls, source_library_id): """ - Dynamically selects block_ids indicating which of the possible children are displayed to the current user. - - Arguments: - selected - list of (block_type, block_id) tuples assigned to this student - children - children of this block - max_count - number of components to display to each student - mode - how content is drawn from the library - - Returns: - A dict containing the following keys: - - 'selected' (set) of (block_type, block_id) tuples assigned to this student - 'invalid' (set) of dropped (block_type, block_id) tuples that are no longer valid - 'overlimit' (set) of dropped (block_type, block_id) tuples that were previously selected - 'added' (set) of newly added (block_type, block_id) tuples - """ - rand = random.Random() - - selected_keys = {tuple(k) for k in selected} # set of (block_type, block_id) tuples assigned to this student - - # Determine which of our children we will show: - valid_block_keys = {(c.block_type, c.block_id) for c in children} - - # Remove any selected blocks that are no longer valid: - invalid_block_keys = (selected_keys - valid_block_keys) - if invalid_block_keys: - selected_keys -= invalid_block_keys - - # If max_count has been decreased, we may have to drop some previously selected blocks: - overlimit_block_keys = set() - if len(selected_keys) > max_count: - num_to_remove = len(selected_keys) - max_count - overlimit_block_keys = set(rand.sample(selected_keys, num_to_remove)) - selected_keys -= overlimit_block_keys - - # Do we have enough blocks now? - num_to_add = max_count - len(selected_keys) - - added_block_keys = None - if num_to_add > 0: - # We need to select [more] blocks to display to this user: - pool = valid_block_keys - selected_keys - if mode == "random": - num_to_add = min(len(pool), num_to_add) - added_block_keys = set(rand.sample(pool, num_to_add)) - # We now have the correct n random children to show for this user. - else: - raise NotImplementedError("Unsupported mode.") - selected_keys |= added_block_keys - - if any((invalid_block_keys, overlimit_block_keys, added_block_keys)): - selected = list(selected_keys) - random.shuffle(selected) + A static method for the implementation of source_library_key + For use in block transformers, which don't have access to non-static methods. + """ + try: + return LibraryLocator.from_string(source_library_id) + except InvalidKeyError: + return LibraryLocatorV2.from_string(source_library_id) - return { - 'selected': selected, - 'invalid': invalid_block_keys, - 'overlimit': overlimit_block_keys, - 'added': added_block_keys, - } + @property + def non_editable_metadata_fields(self): + """ + This variable contains a list of the XBlock fields that should not be displayed in the Studio editor. + """ + non_editable_fields = super().non_editable_metadata_fields + non_editable_fields.extend([ + LibraryContentBlock.source_library_version, + LibraryContentBlock.candidates, + ]) + return non_editable_fields def _publish_event(self, event_name, result, **kwargs): """ @@ -257,7 +251,7 @@ def _publish_event(self, event_name, result, **kwargs): event_data = { "location": str(self.location), "result": result, - "previous_count": getattr(self, "_last_event_result_count", len(self.selected)), + "previous_count": getattr(self, "_last_event_result_count", len(self.selected_block_keys)), "max_count": self.max_count, } event_data.update(kwargs) @@ -265,7 +259,186 @@ def _publish_event(self, event_name, result, **kwargs): self._last_event_result_count = len(result) # pylint: disable=attribute-defined-outside-init @classmethod - def publish_selected_children_events(cls, block_keys, format_block_keys, publish_event): + def _derive_child_block_key(cls, lcb_usage_key: UsageKey, library_block_usage_key: UsageKey) -> BlockKey: + """ + Compute the appropriate block key for the child of a LibraryContentBlock (aka LCB) + that is sourced from a certain library block. + + That is, given they keys of LibraryContentBlock and LibBlock2, we want to find the key of CHILD: + + Course Library + | | + V | + ... | + | | + v LibBlock1 <--| + LibraryContentBlock | + | | + | + - - - LibBlock2 <--| + v . | + CHILD < - - - - + | + LibBlock3 <--+ + """ + # Historically, BlockKeys for children have been generated using V1 Library keys (LibraryLocators). + # As we migrate V1 libraries to V2 libraries, we must keep the derived BlockKeys stable in order to maintain + # student state across the migration. + # So, for V2 libraries, we actually convert the V2 library key back into an "equivalent" V1 library key. + # We will have to maintain this historical artifact even after V1 libraries are deprecated and removed. + # TODO: Confirm that we still want to migrate V1->V2 libraries in-place like this + # (https://github.com/openedx/edx-platform/issues/33640). + true_source_context = library_block_usage_key.context_key + derivable_source_context: LibraryLocator + if isinstance(true_source_context, LibraryLocator): + derivable_source_context = true_source_context + elif isinstance(true_source_context, LibraryLocatorV2): + derivable_source_context = LibraryLocator( + true_source_context.org, # type: ignore[abstract] + true_source_context.slug, + ) + else: + raise TypeError( + f"Source context for '{library_block_usage_key}' is '{true_source_context}'. " + f"Expected source context to be 'library-v1:' (a V1 library) or 'lib:' (a V2 library)." + ) + source_block = BlockKey.from_usage_key(library_block_usage_key) + derivable_source_usage = derivable_source_context.make_usage_key(*source_block) + dest_parent_block = BlockKey.from_usage_key(lcb_usage_key) + return derive_key(source=derivable_source_usage, dest_parent=dest_parent_block) + + def available_children(self) -> list[BlockKey]: + """ + Returns an ordered list of LCB child BlockKeys which are possible for selection, based on either: + * this LCB's candidates (if manual), or + * this LCB's full children list (if not). + + In the manual case, we filter out any candidates which do not actually map to a child of this LCB, + which could happen in the case of a poorly-formed OLX import. + """ + return self.get_available_children( + usage_key=self.location, + all_children=self.children, + candidates=[UsageKey.from_string(candidate) for candidate in self.candidates], + manual=self.manual, + ) + + @classmethod + def _get_candidates_as_children(cls, lcb_usage_key: UsageKey, candidates: list[UsageKey]) -> list[BlockKey]: + return [ + cls._derive_child_block_key(lcb_usage_key=lcb_usage_key, library_block_usage_key=candidate) + for candidate in candidates + ] + + @classmethod + def get_available_children( + cls, + usage_key: UsageKey, + all_children: list[UsageKey], + candidates: list[UsageKey], + manual: bool, + ) -> list[BlockKey]: + """ + Static implementation of available_children. + """ + all_children_block_keys = [BlockKey(child.block_type, child.block_id) for child in all_children] + if manual: + return [ + child + for child in cls._get_candidates_as_children(lcb_usage_key=usage_key, candidates=candidates) + if child in all_children_block_keys + ] + else: + return all_children_block_keys + + @classmethod + def make_selection( + cls, + usage_key: UsageKey, + old_selected: list[BlockKey], + all_children: list[UsageKey], + candidates: list[UsageKey], + max_count: int, + manual: bool, + shuffle: bool, + ) -> dict[str, list[BlockKey]]: + """ + Dynamically selects block_ids indicating which of the possible children are displayed to the current user. + The blocks returned are kept consistent for a user, + unless changes have been made to the library's contents or the settings of the block. + Returns: + A dict containing the following keys: + 'selected': ordered list of BlockKeys assigned to this student + 'invalid': unordered list of BlockKeys that were dropped because they're no longer available + 'overlimit': unordered list of BlockKeys that were dropped because they no longer fit + 'added': unordered list of newly-added BlockKeys + + When generating randomized content to show to a user, we want the following user experience: + 1. When a learner first interacts with the block, they are shown $max_count items from the available children + at random, or all available children if $max_count is -1. If $shuffle is enabled, the order is random. + If $shuffle is disabled, then the order is author-determined. + 2. Every subsequent time they view that content, they are given the same content in the same order, + unless one or more of the below conditions is met: + A. The max_count is increased, requiring the learner to see more. + B. The max_count is decreased, requiring the learner to see less. + C. Blocks previously assigned to a learner become unavailable (via deletion or removal from candidates). + D. max_count is -1 and more blocks become available, requiring the learner to see those new blocks. + 3. In case A, we take the selected blocks that were valid before the edit, + and supplement those with new blocks chosen at random until the number of blocks is $max_count. + 4. In case B, we take the selected blocks that were valid before the edit, and remove blocks randomly + until he number of blocks is <= $max_count. + 5. In case C, we remove the blocks that are now unavailable, and supplement those remaining with new blocks + chosen at random until the number of blocks is $max_count. + 6. In case D, we supplement the selected blocks with the newly-available blocks. + 7. In all cases A-D, if $shuffle is enabled, then the order is re-randomized. + """ + selected: list[BlockKey] = old_selected.copy() + available: list[BlockKey] = cls.get_available_children( + usage_key=usage_key, + all_children=all_children, + candidates=candidates, + manual=manual, + ) + + # Remove blocks from selection if they're no longer candidates/children. + selected = [block for block in selected if block in available] + invalid: set[BlockKey] = set(old_selected) - set(selected) + + # Remove or add blocks if we don't have the correct number in the selection. + additions: set[BlockKey] = set() + overlimit: set[BlockKey] = set() + desired_size: int = min(max_count, len(available)) if max_count >= 0 else len(available) + if len(selected) > desired_size: + num_to_remove = len(selected) - desired_size + overlimit = set(random.sample(selected, num_to_remove)) + selected = [block for block in selected if block not in overlimit] + elif len(selected) < desired_size: + num_to_add = desired_size - len(selected) + available_additions: set[BlockKey] = set(available) - set(selected) + additions = set(random.sample(available_additions, num_to_add)) + selected = [block for block in available if block in (additions | set(selected))] + + # If we've made any change AND if shuffling is enabled, then re-shuffle. + # In other words, always shuffle UNLESS: + # * no changes have been made (because we don't want to re-order a learner's blocks for no reason); OR + # * shuffling is disabled (the code above ensures that we're using the order from the library). + # Otherwise, use the pre-existing order. + if shuffle and (invalid or overlimit or additions): + random.shuffle(selected) + + # return lists because things get json serialized down the line. + return { + 'selected': selected, + 'invalid': list(invalid), + 'overlimit': list(overlimit), + 'added': list(additions), + } + + @classmethod + def publish_selected_children_events( + cls, + block_keys: dict[str, list[BlockKey]], + format_block_keys: Callable[[list[BlockKey]], list], + publish_event: Callable[..., None], + ) -> None: """ Helper method for publishing events when children blocks are selected/updated for a user. This helper is also used by @@ -319,22 +492,39 @@ def publish_selected_children_events(cls, block_keys, format_block_keys, publish added=format_block_keys(block_keys['added']) ) - def selected_children(self): + @property + def selected_block_keys(self) -> list[BlockKey]: + """ + Same as self.selected, but converted from JSON-friendly 2-element-lists into typing-friendly BlockKeys. + """ + return [BlockKey(block_type, block_id) for block_type, block_id in self.selected] + + @selected_block_keys.setter + def selected_block_keys(self, value: list[BlockKey]) -> None: + """ + Convert BlockKeys back into 2-element-lists. + """ + self.selected = [[block_type, block_id] for block_type, block_id in value] + + def selected_children(self) -> list[BlockKey]: """ Returns a [] of block_ids indicating which of the possible children have been selected to display to the current user. This reads and updates the "selected" field, which has user_state scope. - - Note: the return value (self.selected) contains block_ids. To get - actual BlockUsageLocators, it is necessary to use self.children, - because the block_ids alone do not specify the block type. """ max_count = self.max_count if max_count < 0: max_count = len(self.children) - - block_keys = self.make_selection(self.selected, self.children, max_count, "random") # pylint: disable=no-member + block_keys = self.make_selection( + usage_key=self.location, + old_selected=self.selected_block_keys, + all_children=self.children, + candidates=[UsageKey.from_string(candidate) for candidate in self.candidates], + max_count=self.max_count, + manual=self.manual, + shuffle=self.shuffle, + ) # Publish events for analytics purposes: lib_tools = self.get_tools() @@ -348,9 +538,9 @@ def selected_children(self): if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')): # Save our selections to the user state, to ensure consistency: selected = block_keys['selected'] - self.selected = selected # TODO: this doesn't save from the LMS "Progress" page. + self.selected_block_keys = selected # TODO: this doesn't save from the LMS "Progress" page. - return self.selected + return self.selected_block_keys @XBlock.handler def reset_selected_children(self, _, __): @@ -370,7 +560,7 @@ def reset_selected_children(self, _, __): block.reset_problem(None) block.save() - self.selected = [] + self.selected_block_keys = [] return Response(json.dumps(self.student_view({}).content)) def _get_selected_child_blocks(self): @@ -382,6 +572,9 @@ def _get_selected_child_blocks(self): yield self.runtime.get_block(self.location.course_key.make_usage_key(block_type, block_id)) def student_view(self, context): # lint-amnesty, pylint: disable=missing-function-docstring + """ + Renders the view that learners see. + """ fragment = Fragment() contents = [] child_context = {} if not context else copy(context) @@ -436,15 +629,11 @@ def author_view(self, context): max_count = self.max_count if max_count < 0: max_count = len(self.children) - - fragment.add_content(self.runtime.service(self, 'mako').render_cms_template( - "library-block-author-preview-header.html", { - 'max_count': max_count, - 'display_name': self.display_name or self.url_name, - })) + context = {} if not context else copy(context) context['can_edit_visibility'] = False context['can_move'] = False context['can_collapse'] = True + context['selectable'] = True self.render_children(context, fragment, can_reorder=False, can_add=False) # else: When shown on a unit page, don't show any sort of preview - # just the status of this block in the validation area. @@ -457,10 +646,10 @@ def author_view(self, context): def studio_view(self, _context): """ - Return the studio view. + Render a form for editing this XBlock """ fragment = Fragment( - self.runtime.service(self, 'mako').render_cms_template(self.mako_template, self.get_context()) + self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/library_content_edit_helpers.js')) add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor') @@ -473,19 +662,7 @@ def get_child_blocks(self): """ return list(self._get_selected_child_blocks()) - @property - def non_editable_metadata_fields(self): - non_editable_fields = super().non_editable_metadata_fields - # The only supported mode is currently 'random'. - # Add the mode field to non_editable_metadata_fields so that it doesn't - # render in the edit form. - non_editable_fields.extend([ - LibraryContentBlock.mode, - LibraryContentBlock.source_library_version, - ]) - return non_editable_fields - - def get_tools(self, to_read_library_content: bool = False) -> 'LibraryToolsService': + def get_tools(self, to_read_library_content: bool = False) -> LibraryToolsService: """ Grab the library tools service and confirm that it'll work for us. Else, raise LibraryToolsUnavailable. """ @@ -506,6 +683,38 @@ def get_user_id(self): user_id = None return user_id + def render_children( + self, + context: dict[str, Any], + fragment: Fragment, + can_reorder: bool = False, + can_add: bool = False, + ) -> None: + """ + Renders the children of the module with HTML appropriate for Studio. If can_reorder is True, + then the children will be rendered to support drag and drop. + """ + contents = [] + for child in self.get_children(): # pylint: disable=no-member + if can_reorder: + context['reorderable_items'].add(child.location) + context['can_add'] = can_add + + context['is_selected'] = [child.location.block_type, child.location.block_id] in self.candidates + rendered_child = child.render(StudioEditableBlock.get_preview_view_name(child), context) + fragment.add_fragment_resources(rendered_child) + contents.append({ + 'id': str(child.location), + 'content': rendered_child.content + }) + + fragment.add_content(self.runtime.service(self, 'mako').render_template("studio_render_children_view.html", { # pylint: disable=no-member + 'items': contents, + 'xblock_context': context, + 'can_add': can_add, + 'can_reorder': can_reorder, + })) + def _validate_sync_permissions(self): """ Raises PermissionDenied() if we can't confirm that user has write on this block and read on source library. @@ -520,6 +729,25 @@ def _validate_sync_permissions(self): if not user_perms.can_read(self.source_library_key): raise PermissionDenied(f"Cannot read library at {self.source_library_key}") + @XBlock.handler + def get_block_ids(self, request, suffix=''): # lint-amnesty, pylint: disable=unused-argument + """ + Return candidates for selection, represented as usage keys of this LCB's children. + """ + return Response( + json.dumps( + { + 'candidates': [ + str(self.location.context_key.make_usage_key(*block_key)) + for block_key in self._get_candidates_as_children( + lcb_usage_key=self.location, + candidates=[UsageKey.from_string(candidate) for candidate in self.candidates], + ) + ], + } + ) + ) + @XBlock.handler def upgrade_and_sync(self, request=None, suffix=None): # pylint: disable=unused-argument """ @@ -726,7 +954,8 @@ def source_library_values(self): def post_editor_saved(self, user, old_metadata, old_content): # pylint: disable=unused-argument """ - If source library or capa_type have been edited, upgrade library & sync automatically. + If source library, library version or capa_type have been edited, upgrade library, + clear the candidates & sync automatically. TODO: capa_type doesn't really need to trigger an upgrade once we've migrated to V2. """ @@ -735,6 +964,7 @@ def post_editor_saved(self, user, old_metadata, old_content): # pylint: disable if source_lib_changed or capa_filter_changed: try: self.sync_from_library(upgrade_to_latest=True) + self.candidates = [] except (ObjectDoesNotExist, LibraryToolsUnavailable): # The validation area will display an error message, no need to do anything now. pass @@ -742,7 +972,7 @@ def post_editor_saved(self, user, old_metadata, old_content): # pylint: disable def has_dynamic_children(self): """ Inform the runtime that our children vary per-user. - See get_child_blocks() above + See get_child_blocks() """ return True @@ -761,6 +991,9 @@ def get_content_titles(self): @classmethod def definition_from_xml(cls, xml_object, system): + """ + parses the definition and child objects from a piece of xml, the storage format for xblocks. + """ children = [] for child in xml_object.getchildren(): diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index fd4b97fd546e..0d7c53851175 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -3,19 +3,24 @@ Higher-level tests are in `cms/djangoapps/contentstore/tests/test_libraries.py`. """ +from __future__ import annotations + +import itertools from unittest.mock import MagicMock, Mock, patch import ddt from bson.objectid import ObjectId from fs.memoryfs import MemoryFS from lxml import etree +from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 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 openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx.core.djangolib.testing.utils import skip_unless_cms, skip_unless_lms +from xmodule import library_content_block from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LibraryContentBlock from xmodule.library_tools import LibraryToolsService from xmodule.modulestore import ModuleStoreEnum @@ -25,6 +30,7 @@ from xmodule.validation import StudioValidationMessage from xmodule.x_module import AUTHOR_VIEW from xmodule.capa_block import ProblemBlock +from xmodule.util.keys import BlockKey from common.djangoapps.student.tests.factories import UserFactory from .test_course_block import DummySystem as TestImportSystem @@ -32,8 +38,7 @@ dummy_render = lambda block, _: Fragment(block.data) # pylint: disable=invalid-name -@skip_unless_cms -class LibraryContentTest(MixedSplitTestCase): +class LibraryContentTestMixin: """ Base class for tests of LibraryContentBlock (library_content_block.py) """ @@ -87,8 +92,9 @@ def get_block(descriptor): block.runtime.get_block_for_descriptor = get_block +@skip_unless_cms @ddt.ddt -class LibraryContentGeneralTest(LibraryContentTest): +class LibraryContentGeneralTest(LibraryContentTestMixin, MixedSplitTestCase): """ Test the base functionality of the LibraryContentBlock. """ @@ -133,7 +139,8 @@ def test_initial_sync_from_library(self): assert len(self.lc_block.children) == len(self.lib_blocks) -class TestLibraryContentExportImport(LibraryContentTest): +@skip_unless_cms +class TestLibraryContentExportImport(LibraryContentTestMixin, MixedSplitTestCase): """ Export and import tests for LibraryContentBlock """ @@ -173,7 +180,8 @@ def _verify_xblock_properties(self, imported_lc_block): 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.mode == self.lc_block.mode + assert imported_lc_block.shuffle == self.lc_block.shuffle + assert imported_lc_block.manual == self.lc_block.manual 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) @@ -424,8 +432,8 @@ def test_non_editable_settings(self): Test the settings that are marked as "non-editable". """ non_editable_metadata_fields = self.lc_block.non_editable_metadata_fields - assert LibraryContentBlock.mode in non_editable_metadata_fields assert LibraryContentBlock.display_name not in non_editable_metadata_fields + assert LibraryContentBlock.source_library_version in non_editable_metadata_fields def test_overlimit_blocks_chosen_randomly(self): """ @@ -502,8 +510,9 @@ def test_reset_selected_children_capa_blocks(self, allow_resetting_children, max search_index_mock = Mock(spec=SearchEngine) # pylint: disable=invalid-name +@skip_unless_cms @patch.object(SearchEngine, 'get_search_engine', Mock(return_value=None, autospec=True)) -class TestLibraryContentBlockWithSearchIndex(LibraryContentBlockTestMixin, LibraryContentTest): +class TestLibraryContentBlockWithSearchIndex(LibraryContentBlockTestMixin, LibraryContentTestMixin, MixedSplitTestCase): """ Tests for library container with mocked search engine response. """ @@ -527,12 +536,13 @@ def setUp(self): search_index_mock.search = Mock(side_effect=self._get_search_response) +@skip_unless_cms @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 TestLibraryContentRender(LibraryContentTest): +class TestLibraryContentRender(LibraryContentTestMixin, MixedSplitTestCase): """ Rendering unit tests for LibraryContentBlock """ @@ -559,7 +569,8 @@ def test_author_view(self): # but some js initialization should happen -class TestLibraryContentAnalytics(LibraryContentTest): +@skip_unless_cms +class TestLibraryContentAnalytics(LibraryContentTestMixin, MixedSplitTestCase): """ Test analytics features of LibraryContentBlock """ @@ -736,3 +747,624 @@ 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' + + +def _mock_selection_shuffle(wrapped): + """ + Replace "shuffling" with simply "reversing". + + That is, make it so that when `shuffle==True`, the order of a learner's selection is just the + order of the learner's children, backwards. + + Reversing simulates shuffling well enough for our purposes, and it gives us two nice guarantees: + * Selection order is stable. This makes it easier to debug test failures, and lowers the risk of test flakiness. + * When `len(selection) >= 2` and `shuffle=True`, the selection order will always be different than order of the + LCB's children. This avoids false-positive situations, wherein a test passes only because the shuffled selection + happened to be the same order as the original children. + """ + def _mock_shuffle(selected: list): + selected.reverse() + return patch.object(library_content_block.random, "shuffle", _mock_shuffle)(wrapped) + + +def _mock_selection_sample(wrapped): + """ + Use a fake, deterministic "random sample" algorithm for when the selection must be build from a random subset of + the children/candidates. To maximize test stability, input order does not matter, but output order is stable. + """ + def _mock_sample(pool, count: int) -> list: + """ + Until count is reached, pick sample one-at-a-time from sorted pool using this pattern: + last, 1st, 2nd-to-last, 2nd, 3rd-to-last, 3rd, 4th-to-last, 4th, etc. + For example: + random.sample(['b','c','a','e','d'], 4) + == random.sample(['a','b','c','d','e'], 4) + == ['e', 'a', 'b', 'd'] + """ + assert count <= len(set(pool)) + sample = [] + remaining = sorted(set(pool)) + while len(sample) < count: + remaining = list(reversed(remaining)) + sample.append(remaining.pop(0)) + assert len(set(sample)) == count # Sanity check our algorithm + return sample + return patch.object(library_content_block.random, "sample", _mock_sample)(wrapped) + + +def _mock_child_key_derivation(wrapped): + """ + Generate LCB child keys in a way that allows us to write readable unit tests. + """ + def _mock_derive_key(source: UsageKey, dest_parent: BlockKey) -> BlockKey: + """ + Instead of making a hash digest, just smash the source+dest key parts together with underscores. + """ + source_library = source.context_key + return BlockKey( + source.block_type, + f"{source.context_key.org}_{source.context_key.library}_{source.block_id}_{dest_parent.id}", + ) + return patch.object(library_content_block, "derive_key", _mock_derive_key)(wrapped) + + +@skip_unless_lms +@ddt.ddt +@_mock_selection_shuffle +@_mock_selection_sample +@_mock_child_key_derivation +class TestLibraryContentSelection(MixedSplitTestCase): + """ + Test library content selection for the various modes of the LibraryContentBlock. + + This is tested entirely using LMS features, so no actual Content Libraries are created or synced from. + Instead, we create a LibraryContentBlock (LCB) and add/remove children. + This simulates how the CMS would add/remove children as part of the sync_from_library process. + """ + def setUp(self): + super().setUp() + self.user_id = UserFactory().id + self.course = CourseFactory.create(modulestore=self.store) + chapter = self.make_block("chapter", self.course) + sequential = self.make_block("sequential", chapter) + vertical = self.make_block("vertical", sequential) + self.lc_block = self.make_block("library_content", vertical) + self.source_library_key = LibraryLocator.from_string("library-v1:myorg+mylib") + + def _create_lc_block_children(self, block_ids: list[str]) -> None: + """ + Add HTML blocks as children of the LCB, simluating an item being added to the source library. + + Each item should be the .block_id part of a UsageKey. + """ + selected = self.lc_block.selected # TODO/HACK: 'selected' is lost upon re-load, so we manuallay save+fix it. + self.store.update_item(self.lc_block, self.user_id) # Persist any changes so we can reload later. + for block_id in block_ids: + new_block = self.make_block(category="html", parent_block=self.lc_block, display_name=block_id) + self.lc_block = self.store.get_item(self.lc_block.location) # Reload to update '.children'. + self.lc_block.selected = selected # TODO/HACK + + def _set_lc_block_candidates(self, block_ids: list[str]) -> None: + """ + Set HTML blocks as the candiates of the LCB. + """ + self.lc_block.candidates = [ + str(self.source_library_key.make_usage_key("html", block_id)) for block_id in block_ids + ] + + def _remove_lc_block_child(self, block_key: tuple[str, str], delete_child=True, remove_candidate=True) -> None: + """ + Simulate the removal of a block from the source library and/or the candidates list. + + If `delete_child`, remove it from the LCB's list of children and from modulestore entirely. + If `remove_candidate`, remove it fromthe LCB's list of candidates. + """ + assert delete_child or remove_candidate + if remove_candidate: + # candidates may contain tuples or lists, depending on whether it's been + # loaded from modulestore recently. Handle both cases. + try: + self.lc_block.candidates.remove(tuple(block_key)) + except ValueError: + assert list(block_key) in self.lc_block.candidates + self.lc_block.candidates.remove(list(block_key)) + if delete_child: + block_usage_key = self.course.id.make_usage_key(*block_key) + assert block_usage_key in self.lc_block.children + selected = self.lc_block.selected # TODO/HACK: 'selected' is lost upon reload, so we manually save+fix it. + self.store.update_item(self.lc_block, self.user_id) # Persist any changes so we can reload later. + self.store.delete_item(block_usage_key, self.user_id) + self.lc_block = self.store.get_item(self.lc_block.location) # Reload to update '.children'. + self.lc_block.selected = selected # TODO/HACK + + @ddt.data( + dict( + # "Randomized mode" with empty library. + manual=False, + shuffle=True, + children=[], + candidates=[], + ), + dict( + # "Randomized mode" with empty library. There are fake entries in candidates, which shouldn't matter. + manual=False, + shuffle=False, # disabling shuffling -- shouldn't affect anything. + children=[], + candidates=['i-do-not-exist', 'i-am-not-real'], + ), + dict( + # "Static mode" with library items but no candidates. + manual=True, + shuffle=False, + children=['a', 'b', 'c'], + candidates=[], + ), + dict( + # "Static mode with shuffling" with library items and candidates, but the candidates don't actually exist. + manual=True, + shuffle=True, + children=['a', 'b', 'c'], + candidates=['i-do-not-exist', 'i-am-not-real'], + ), + ) + @ddt.unpack + def test_none_available_for_selection(self, manual, shuffle, children, candidates): + """ + Test various scennarios in which there are no blocks available to be selected. + """ + self.lc_block.manual = manual + self.lc_block.shuffle = shuffle + self._create_lc_block_children(children) + self._set_lc_block_candidates(candidates) + assert self.lc_block.available_children() == [] + assert self.lc_block.selected_children() == [] + + @ddt.data( + dict( + # "Randomized mode" + manual=False, + shuffle=True, + initial_children=['a', 'b', 'c', 'd'], + initial_candidates=[], + ), + dict( + # "Static mode" (with one non-candidate child) + manual=True, + shuffle=False, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + ), + dict( + # "Static mode with shuffling" (with one non-candidate child) + manual=True, + shuffle=True, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + ), + ) + @ddt.unpack + def test_expanding_selection(self, manual, shuffle, initial_children, initial_candidates): + """ + Test that increasing max_count and/or available children results in an expanded selection. + """ + # Start with 4 available blocks. + self.lc_block.manual = manual + self.lc_block.shuffle = shuffle + self._create_lc_block_children(initial_children) + self._set_lc_block_candidates(initial_candidates) + assert len(self.lc_block.available_children()) == 4 # Sanity check ddt input + + # Start with 2 selected. + self.lc_block.max_count = 2 + selection_of_2 = self.lc_block.selected_children() + assert len(selection_of_2) == 2 + + # Increase to 3 selected. Original 2 should remain selected. + self.lc_block.max_count = 3 + selection_of_3 = self.lc_block.selected_children() + assert len(selection_of_3) == 3 + assert set(selection_of_2) < set(selection_of_3) + + # Increase to -1 (all 4 blocks selected). The original 3 should remain selected. + self.lc_block.max_count = -1 + selection_of_all_4 = self.lc_block.selected_children() + assert len(selection_of_all_4) == 4 + assert set(selection_of_3) < set(selection_of_all_4) + if shuffle: + assert set(selection_of_all_4) == set(self.lc_block.available_children()) + else: + assert selection_of_all_4 == self.lc_block.available_children() + + # Toss a new block into the children list, and add it to candidates list too. + additional_child = 'e' + self._create_lc_block_children([additional_child]) + self.lc_block.candidates += [additional_child] + + # MANUAL ONLY: toss another non-candidate block into the children -- should be ignored. + if manual: + self._create_lc_block_children(['y']) # Another additional non-candidate child + + # Since -1 means "use all available children", 5 should be selected, including the 4 from last step. + assert len(self.lc_block.available_children()) == 5 + selection_of_all_5 = self.lc_block.selected_children() + assert len(selection_of_all_5) == 5 + assert set(selection_of_all_4) < set(selection_of_all_5) + if shuffle: + assert set(selection_of_all_5) == set(self.lc_block.available_children()) + else: + assert selection_of_all_5 == self.lc_block.available_children() + + # Increase max_count past the number of available children. + # Since "too many" is treated the same as "all", the selection should be unchanged. + # Even when shuffle==True, the order should be unchanged, since the selected set is unchanged. + self.lc_block.max_count = 10 + assert self.lc_block.selected_children() == selection_of_all_5 + + @ddt.data( + dict( + # "Randomized mode" + manual=False, + shuffle=True, + initial_children=['a', 'b', 'c', 'd'], + initial_candidates=[], + ), + dict( + # "Static mode" (with one non-candidate child) + manual=True, + shuffle=False, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + ), + dict( + # "Static mode with shuffling" (with one non-candidate child) + manual=True, + shuffle=True, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + ), + ) + @ddt.unpack + def test_overlimit_selection(self, manual, shuffle, initial_children, initial_candidates): + """ + Test that decreasing the max_count value leads a reduced version of the original selection. + """ + # Start with 4 available blocks. + self.lc_block.manual = manual + self.lc_block.shuffle = shuffle + self._create_lc_block_children(initial_children) + self._set_lc_block_candidates(initial_candidates) + assert len(self.lc_block.available_children()) == 4 # Sanity check ddt input + + # Start with max selection. + self.lc_block.max_count = -1 + selection_of_all_4 = set(self.lc_block.selected_children()) + assert len(selection_of_all_4) == 4 + assert selection_of_all_4 == set(self.lc_block.available_children()) + + # Then drop it down to 3... should be a subset. + self.lc_block.max_count = 3 + selection_of_all_3 = set(self.lc_block.selected_children()) + assert len(selection_of_all_3) == 3 + assert selection_of_all_3 < selection_of_all_4 + + # Then drop it down to 2... should be a smaller subset. + self.lc_block.max_count = 2 + selection_of_all_2 = set(self.lc_block.selected_children()) + assert len(selection_of_all_2) == 2 + assert selection_of_all_2 < selection_of_all_3 + + @ddt.data( + dict( + # "Randomized mode" + manual=False, + shuffle=True, + index_of_selected_to_keep=0, + initial_children=['a', 'b', 'c', 'd'], + initial_candidates=[], + delete_child=True, + remove_candidate=False, + ), + dict( + # "Randomized mode", but we also remove from the candidates list (which should have no extra effect) + manual=False, + shuffle=True, + index_of_selected_to_keep=1, + initial_children=['a', 'b', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + delete_child=True, + remove_candidate=True, + ), + dict( + # "Static mode" (with one non-candidate child). + manual=True, + shuffle=False, + index_of_selected_to_keep=0, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + delete_child=False, + remove_candidate=True, + ), + dict( + # "Static mode with shuffling" (with one non-candidate child). + # TWIST: Remove from underling lib, not candidates. Should have the same effect of removing from candidates. + manual=True, + shuffle=True, + index_of_selected_to_keep=1, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + delete_child=True, + remove_candidate=False, + ), + ) + @ddt.unpack + def test_unavailable_block_with_replacement( + self, + manual, + shuffle, + initial_children, + initial_candidates, + index_of_selected_to_keep, + delete_child, + remove_candidate, + ): + """ + Test that if a selected block becomes unavailable (either by library removal or candidate removal) + when there are ARE other replacement blocks available, then it is just replaced with one of those. + Other selected blocks should remain selected. + """ + # Start with 4 available blocks. + self.lc_block.manual = manual + self.lc_block.shuffle = shuffle + self._create_lc_block_children(initial_children) + self._set_lc_block_candidates(initial_candidates) + assert len(self.lc_block.available_children()) == 4 # Sanity check ddt input + + # Start with 2 selected blocks. + self.lc_block.max_count = 2 + initial_selection = self.lc_block.selected_children() + assert len(initial_selection) == 2 + + # Keep one, remove one. + selected_child_to_keep = initial_selection[index_of_selected_to_keep] + (selected_child_to_remove,) = set(initial_selection) - {selected_child_to_keep} + self._remove_lc_block_child( + selected_child_to_remove, delete_child=delete_child, remove_candidate=remove_candidate + ) + assert len(self.lc_block.available_children()) == 3 + + # New selection should still have 2 blocks: the kept block, and another lib block + new_selection = self.lc_block.selected_children() + assert len(new_selection) == 2 + assert selected_child_to_keep in new_selection + assert selected_child_to_remove not in new_selection + + @ddt.data( + dict( + # "Randomized mode" + manual=False, + shuffle=True, + index_of_selected_to_keep=0, + initial_children=['a', 'b', 'c', 'd'], + initial_candidates=[], + delete_child=True, + remove_candidate=False, + ), + dict( + # "Randomized mode", but we also remove from the candidates list (which should have no extra effect) + manual=False, + shuffle=True, + index_of_selected_to_keep=1, + initial_children=['a', 'b', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + delete_child=True, + remove_candidate=True, + ), + dict( + # "Static mode" (with one non-candidate child). + manual=True, + shuffle=False, + index_of_selected_to_keep=0, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + delete_child=False, + remove_candidate=True, + ), + dict( + # "Static mode with shuffling" (with one non-candidate child). + # TWIST: Remove from underling lib, not candidates. Should have the same effect of removing from candidates. + manual=True, + shuffle=True, + index_of_selected_to_keep=1, + initial_children=['a', 'b', 'x', 'c', 'd'], + initial_candidates=['a', 'b', 'c', 'd'], + delete_child=True, + remove_candidate=False, + ), + ) + @ddt.unpack + def test_unavilable_block_without_replacement( + self, + manual, + shuffle, + initial_children, + initial_candidates, + index_of_selected_to_keep, + delete_child, + remove_candidate, + ): + """ + Test that if a selected block becomes unavailable (either by library removal or candidate removal) + when there are ARE other replacement blocks available, then it is just replaced with one of those. + Other selected blocks should remain selected. + """ + # Start with 4 available blocks. + self.lc_block.manual = manual + self.lc_block.shuffle = shuffle + self._create_lc_block_children(initial_children) + self._set_lc_block_candidates(initial_candidates) + assert len(self.lc_block.available_children()) == 4 # Sanity check ddt input + + # Start with 2 selected blocks. + self.lc_block.max_count = 2 + initial_selection = self.lc_block.selected_children() + assert len(initial_selection) == 2 + + # Choose just one of them to keep; remove all other children. + selected_child_to_keep = initial_selection[index_of_selected_to_keep] + for child in self.lc_block.available_children(): + if child != selected_child_to_keep: + self._remove_lc_block_child( + child, delete_child=delete_child, remove_candidate=remove_candidate + ) + assert len(self.lc_block.available_children()) == 1 + + # New selection should have just the 1 remaining block, even though max_count is still 2 + assert self.lc_block.selected_children() == [selected_child_to_keep] + assert self.lc_block.max_count == 2 + + # Finally, remove that last remaining block, and ensure that the selection is empty. + self._remove_lc_block_child( + selected_child_to_keep, delete_child=delete_child, remove_candidate=remove_candidate + ) + assert self.lc_block.available_children() == [] + assert self.lc_block.selected_children() == [] + assert self.lc_block.max_count == 2 + + @ddt.data(*itertools.product((True, False), (True, False), (0, 1), (0, 1))) + @ddt.unpack + def test_complex_scenario(self, manual, shuffle, index_of_selected_to_keep, index_of_unselected_to_keep): + """ + Test that if blocks are added to the source lib, AND blocks are deleted, AND max_count + changes, then everything works out according to the rules of make_selection. + """ + # Start with 4 available blocks + self.lc_block.manual = manual + self.lc_block.shuffle = shuffle + initial_children = ['a', 'b', 'c', 'd'] + self._create_lc_block_children(initial_children) + initial_candidates = ['a', 'b', 'c', 'd'] + self._set_lc_block_candidates(initial_candidates) + self.lc_block.candidates = initial_children + if manual: + self._create_lc_block_children(['x']) # unavailable child + assert self.lc_block.available_children() == initial_children + + # Start with a selection of 2 + self.lc_block.max_count = 2 + initial_selection = self.lc_block.selected_children() + assert len(initial_selection) == 2 + + # From the selection: keep one, but remove the other from the children. + selected_child_to_keep = initial_selection[index_of_selected_to_keep] + (selected_child_to_remove,) = set(initial_selection) - {selected_child_to_keep} + self._remove_lc_block_child(selected_child_to_remove) + + # Now from the *unselected* children: keep one, and remove the other. + unselected_children = [child for child in initial_children if child not in initial_selection] + unselected_child_to_keep = unselected_children[index_of_unselected_to_keep] + (unselected_child_to_remove,) = set(unselected_children) - {unselected_child_to_keep} + self._remove_lc_block_child(unselected_child_to_remove) + + # Finally, add 2 new children. + additional_children = ['e', 'f'] + self._create_lc_block_children(additional_children) + self.lc_block.candidates += additional_children + + # Sanity check + new_children = self.lc_block.available_children() + assert set(new_children) == {selected_child_to_keep, unselected_child_to_keep, *additional_children} + + # THE TEST: Up the max count to 3 and reselect. + # We expect a selection containing 1 block from the old selection, and 2 new ones. + self.lc_block.max_count = 3 + new_selection = self.lc_block.selected_children() + assert len(new_selection) == 3 + still_selected = set(new_selection) & set(initial_selection) + newly_selected = set(new_selection) - set(initial_selection) + assert still_selected == {selected_child_to_keep} + assert len(newly_selected) == 2 + assert newly_selected < {unselected_child_to_keep, *additional_children} + + @ddt.data(True, False) + def test_toggle_manual_when_selecting_all(self, shuffle): + """ + Test the behavior of toggling `manual` on and off when `max_count==-1` (i,e., "select all"). + """ + # 4 children, 2 of which are candidates. + self.lc_block.shuffle = shuffle + children = ['a', 'b', 'c', 'd'] + self._create_lc_block_children(children) + candidates = ['a', 'c'] + self._set_lc_block_candidates(candidates) + + # Select all available. + self.lc_block.max_count = -1 + + # Non-manual mode: All children are selected. + self.lc_block.manual = False + assert self.lc_block.available_children() == children + if shuffle: + assert set(self.lc_block.selected_children()) == set(children) + else: + assert self.lc_block.selected_children() == children + + # Manual mode: All candidates are selected. + self.lc_block.manual = True + assert self.lc_block.available_children() == candidates + if shuffle: + assert set(self.lc_block.selected_children()) == set(candidates) + else: + assert self.lc_block.selected_children() == candidates + + # Back to non-manual mode: All children are selected. + self.lc_block.manual = False + assert self.lc_block.available_children() == children + if shuffle: + assert set(self.lc_block.selected_children()) == set(children) + else: + assert self.lc_block.selected_children() == children + + @ddt.data(True, False) + def test_toggle_manual_when_selecting_subset(self, shuffle): + """ + Test the behavior of toggling `manual` on and off when `max_count` equals the number of candidates, but there + are additional non-candidates. + """ + # 4 children. + self.lc_block.shuffle = shuffle + self.lc_block.max_count = 2 + children = ['a', 'b', 'c', 'd'] + self._create_lc_block_children(children) + + # Non-manual mode: Any 2 of 4 children are selected. + self.lc_block.manual = False + assert self.lc_block.available_children() == children + nonmanual_selected = self.lc_block.selected_children() + assert len(nonmanual_selected) == 2 + assert set(nonmanual_selected) < set(children) + + # Pick 2 candidates: 1 of which is currently selected, and 1 of which is not. + candidates = [nonmanual_selected[0]] + for child in children: + if child not in nonmanual_selected: + candidates.append(child) + break + assert len(candidates) == 2 # Sanity chek + self._set_lc_block_candidates(candidates) + + # Manual mode: Just the 2 candidates should now be selected. + self.lc_block.manual = True + assert self.lc_block.available_children() == candidates + manual_selected = self.lc_block.selected_children() + assert len(manual_selected) == 2 + if shuffle: + assert set(manual_selected) == set(candidates) + else: + assert manual_selected == candidates + + # Back to non-manual mode: + # Selection should be unchanged! Even though all children are now available for selection, + # the selection criteria (max_count==2) is still satisfied, so the selection should remain unchanged. + self.lc_block.manual = False + nonmanual_selected_new = self.lc_block.selected_children() + assert nonmanual_selected_new == manual_selected