diff --git a/mypy.ini b/mypy.ini index 4e69ef616391..c0d739e8468b 100644 --- a/mypy.ini +++ b/mypy.ini @@ -14,7 +14,8 @@ files = openedx/core/djangoapps/xblock, openedx/core/types, openedx/core/djangoapps/content_tagging, - xmodule/util/keys.py + xmodule/util/keys.py, + xmodule/item_bank_block.py [mypy.plugins.django-stubs] # content_staging only works with CMS; others work with either, so we run mypy with CMS settings. diff --git a/setup.py b/setup.py index 21c8e537c97e..3b8f8c59498d 100644 --- a/setup.py +++ b/setup.py @@ -19,6 +19,7 @@ "error = xmodule.error_block:ErrorBlock", "hidden = xmodule.hidden_block:HiddenBlock", "html = xmodule.html_block:HtmlBlock", + "itembank = xmodule.item_bank_block:ItemBankBlock", "image = xmodule.template_block:TranslateCustomTagBlock", "library = xmodule.library_root_xblock:LibraryRoot", "library_content = xmodule.library_content_block:LegacyLibraryContentBlock", diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py new file mode 100644 index 000000000000..e7b1e3bdb0bb --- /dev/null +++ b/xmodule/item_bank_block.py @@ -0,0 +1,500 @@ +""" +Definitions for ItemBankMixin and ItemBankBlock. +""" +from __future__ import annotations + +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 +from lxml.etree import XMLSyntaxError +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 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 ( + ResourceTemplates, + XModuleMixin, + XModuleToXBlockMixin, + shim_xmodule_js, + STUDENT_VIEW, +) + +_ = lambda text: text + +logger = logging.getLogger(__name__) + + +@XBlock.needs('mako') +@XBlock.wants('user') +class ItemBankMixin( + # @@TODO do we really need all these mixins? + MakoTemplateBlockBase, + XmlMixin, + XModuleToXBlockMixin, + ResourceTemplates, + XModuleMixin, + StudioEditableBlock, +): + """ + Shared logic for XBlock types which shows a random subset of their children to each learner. + + Concretely, this is the shared base for ItemBankBlock and LegacyLibraryContentBlock. + Sharing fields and selection logic between those two blocks will allow us to eventually migrate + LegacyLibraryContentBlocks (backed by V1 libraries) into ItemBankBlocks (backed by V2 libraries). + """ + has_children = True + has_author_view = True + show_in_read_only_mode = True + resources_dir = 'assets/library_content' + mako_template = 'widgets/metadata-edit.html' + studio_js_module_name = "VerticalDescriptor" + + 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, + ) + 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 + default=[], + scope=Scope.user_state, + ) + # This cannot be called `show_reset_button`, because children blocks inherit this as a default value. + allow_resetting_children = 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."), + scope=Scope.settings, + default=False + ) + + @classproperty + def completion_mode(cls): # 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): + return XBlockCompletionMode.COMPLETABLE + + return XBlockCompletionMode.AGGREGATOR + + @classmethod + def make_selection(cls, selected, children, max_count): + """ + 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 + + 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(list(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 + num_to_add = min(len(pool), num_to_add) + added_block_keys = set(rand.sample(list(pool), num_to_add)) + selected_keys |= added_block_keys + + if any((invalid_block_keys, overlimit_block_keys, added_block_keys)): + selected = list(selected_keys) + random.shuffle(selected) + + return { + 'selected': selected, + 'invalid': invalid_block_keys, + 'overlimit': overlimit_block_keys, + 'added': added_block_keys, + } + + def _publish_event(self, event_name, result, **kwargs): + """ + Helper method to publish an event for analytics purposes + """ + event_data = { + "location": str(self.location), + "result": result, + "previous_count": getattr(self, "_last_event_result_count", len(self.selected)), + "max_count": self.max_count, + } + event_data.update(kwargs) + self.runtime.publish(self, f"edx.librarycontentblock.content.{event_name}", event_data) + 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): + """ + Helper method for publishing events when children blocks are + selected/updated for a user. This helper is also used by + the ContentLibraryTransformer. + + Arguments: + + block_keys - + A dict describing which events to publish (add or + remove), see `make_selection` above for format details. + + format_block_keys - + A function to convert block keys to the format expected + by publish_event. Must have the signature: + + [(block_type, block_id)] -> T + + Where T is a collection of block keys as accepted by + `publish_event`. + + publish_event - + Function that handles the actual publishing. Must have + the signature: + + <'removed'|'assigned'> -> result:T -> removed:T -> reason:str -> None + + Where T is a collection of block_keys as returned by + `format_block_keys`. + """ + if block_keys['invalid']: + # reason "invalid" means deleted from library or a different library is now being used. + publish_event( + "removed", + result=format_block_keys(block_keys['selected']), + removed=format_block_keys(block_keys['invalid']), + reason="invalid" + ) + + if block_keys['overlimit']: + publish_event( + "removed", + result=format_block_keys(block_keys['selected']), + removed=format_block_keys(block_keys['overlimit']), + reason="overlimit" + ) + + if block_keys['added']: + publish_event( + "assigned", + result=format_block_keys(block_keys['selected']), + added=format_block_keys(block_keys['added']) + ) + + def selected_children(self): + """ + 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) # pylint: disable=no-member + + self.publish_selected_children_events( + block_keys, + self.format_block_keys_for_analytics, + self._publish_event, + ) + + 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. + + 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. + + @@TODO: Do we actually want to share this format between ItemBankBlocks and LegacyLibraryContentBlocks? + Or should we define a fresh format for ItemBankBlocks? + """ + raise NotImplementedError + + @XBlock.handler + def reset_selected_children(self, _, __): + """ + Resets the XBlock's state for a user. + + This resets the state of all `selected` children and then clears the `selected` field + so that the new blocks are randomly chosen for this user. + """ + if not self.allow_resetting_children: + return Response('"Resetting selected children" is not allowed for this XBlock', + status=status.HTTP_400_BAD_REQUEST) + + for block_type, block_id in self.selected_children(): + block = self.runtime.get_block(self.location.course_key.make_usage_key(block_type, block_id)) + if hasattr(block, 'reset_problem'): + block.reset_problem(None) + block.save() + + self.selected = [] + return Response(json.dumps(self.student_view({}).content)) + + def _get_selected_child_blocks(self): + """ + Generator returning XBlock instances of the children selected for the + current user. + """ + for block_type, block_id in self.selected_children(): + yield self.runtime.get_block(self.context_key.make_usage_key(block_type, block_id)) + + def student_view(self, context): # lint-amnesty, pylint: disable=missing-function-docstring + fragment = Fragment() + contents = [] + child_context = {} if not context else copy(context) + + for child in self._get_selected_child_blocks(): + if child is None: + # TODO: Fix the underlying issue in TNL-7424 + # This shouldn't be happening, but does for an as-of-now + # unknown reason. Until we address the underlying issue, + # let's at least log the error explicitly, ignore the + # exception, and prevent the page from resulting in a + # 500-response. + logger.error('Skipping display for child block that is None') + continue + + rendered_child = child.render(STUDENT_VIEW, 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_lms_template('vert_module.html', { + 'items': contents, + 'xblock_context': context, + 'show_bookmark_button': False, + 'watched_completable_blocks': set(), + 'completion_delay_ms': None, + 'reset_button': self.allow_resetting_children, + })) + + fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/library_content_reset.js')) + fragment.initialize_js('LibraryContentReset') + return fragment + + def studio_view(self, _context): + """ + Return the studio view. + """ + fragment = Fragment( + self.runtime.service(self, 'mako').render_cms_template(self.mako_template, self.get_context()) + ) + add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor') + shim_xmodule_js(fragment, self.studio_js_module_name) + return fragment + + def get_child_blocks(self): + """ + Return only the subset of our children relevant to the current student. + """ + return list(self._get_selected_child_blocks()) + + def get_user_id(self): + """ + Get the ID of the current user. + """ + user_service = self.runtime.service(self, 'user') + if user_service: + user_id = user_service.get_current_user().opt_attrs.get('edx-platform.user_id', None) + else: + user_id = None + return user_id + + def has_dynamic_children(self): + """ + Inform the runtime that our children vary per-user. + See get_child_blocks() above + """ + return True + + def get_content_titles(self): + """ + Returns list of friendly titles for our selected children only; without + thi, all possible children's titles would be seen in the sequence bar in + the LMS. + + This overwrites the get_content_titles method included in x_module by default. + """ + titles = [] + for child in self.get_child_blocks(): + titles.extend(child.get_content_titles()) + return titles + + @classmethod + def definition_from_xml(cls, xml_object, system): + """ + @@TODO docstring + """ + children = [] + + for child in xml_object.getchildren(): + try: + children.append(system.process_xml(etree.tostring(child)).scope_ids.usage_id) + except (XMLSyntaxError, AttributeError): + msg = ( + "Unable to load child when parsing {self.usage_key.block_type} Block. " + "This can happen when a comment is manually added to the course export." + ) + logger.error(msg) + if system.error_tracker is not None: + system.error_tracker(msg) + + definition = dict(xml_object.attrib.items()) + return definition, children + + def definition_to_xml(self, resource_fs): + """ Exports ItemBankBlock to XML """ + xml_object = etree.Element(self.usage_key.block_type) + for child in self.get_children(): + self.runtime.add_block_as_child_node(child, xml_object) + # Set node attributes based on our fields. + for field_name, field in self.fields.items(): # pylint: disable=no-member + if field_name in ('children', 'parent', 'content'): + continue + if field.is_set_on(self): + xml_object.set(field_name, str(field.read_from(self))) + return xml_object + + +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. + + @@TODO implement + """ + validation = super().validate() + if not isinstance(validation, StudioValidation): + validation = StudioValidation.copy(validation) + if not validation.empty: + pass # If there's already a validation error, leave it there. + elif not self.children: + validation.set_summary( + StudioValidationMessage( + StudioValidationMessage.WARNING, + (_('No problems have been selected.')), + action_class='edit-button', + action_label=_("Select problems to randomize.") + ) + ) + elif len(self.children) < self.max_count: + validation.set_summary( + StudioValidationMessage( + StudioValidationMessage.WARNING, + _( + "The problem bank has been configured to show {count} problems, " + "but only {actual} have been selected." + ).format(count=self.max_count, actual=len(self.children)), + action_class='edit-button', + action_label=_("Edit the problem bank configuration."), + ) + ) + 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.location == self.location + # User has clicked the "View" link. Show a preview of all possible children: + if is_root and self.children: # pylint: disable=no-member + fragment.add_content(self.runtime.service(self, 'mako').render_cms_template( + "library-block-author-preview-header.html", { + 'max_count': self.max_count if self.max_count >= 0 else len(self.children), + 'display_name': self.display_name or self.url_name, + })) + context['can_edit_visibility'] = False + context['can_move'] = False + context['can_collapse'] = True + self.render_children(context, fragment, can_reorder=False, can_add=False) + context['is_loading'] = False + + fragment.initialize_js('LibraryContentAuthorView') + 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. + + @@TODO this doesn't include original_usage_key, original_usage_version, or descendends! + """ + return [{"usage_key": str(self.context_key.make_usage_key(*block_key))} for block_key in block_keys] diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index adb07101d04c..c2d3aef623d8 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -12,40 +12,22 @@ import json import logging -import random -from copy import copy from gettext import ngettext, gettext import nh3 -from django.conf import settings from django.core.exceptions import ObjectDoesNotExist, PermissionDenied -from django.utils.functional import classproperty -from lxml import etree -from lxml.etree import XMLSyntaxError from opaque_keys.edx.locator import LibraryLocator -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.fields import Integer, Scope, String 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.modulestore.exceptions import ItemNotFoundError +from xmodule.item_bank_block import ItemBankMixin from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.xml_block import XmlMixin -from xmodule.x_module import ( - STUDENT_VIEW, - ResourceTemplates, - XModuleMixin, - XModuleToXBlockMixin, - shim_xmodule_js, -) - -# 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 logger = logging.getLogger(__name__) @@ -80,18 +62,9 @@ def __init__(self): super().__init__("Needed 'library_tools' features which were not available in the current runtime") -@XBlock.wants('library_tools') # TODO: Split this service into its LMS and CMS parts. +@XBlock.wants('library_tools') @XBlock.wants('studio_user_permissions') # Only available in CMS. -@XBlock.wants('user') -@XBlock.needs('mako') -class LegacyLibraryContentBlock( - MakoTemplateBlockBase, - XmlMixin, - XModuleToXBlockMixin, - ResourceTemplates, - XModuleMixin, - StudioEditableBlock, -): +class LegacyLibraryContentBlock(ItemBankMixin, XBlock): """ An XBlock whose children are chosen dynamically from a legacy (v1) content library. Can be used to create randomized assessments among other things. @@ -100,30 +73,6 @@ class LegacyLibraryContentBlock( as children of this block, but only a subset of those children are shown to any particular student. """ - # pylint: disable=abstract-method - has_children = True - has_author_view = True - - resources_dir = 'assets/library_content' - - mako_template = 'widgets/metadata-edit.html' - studio_js_module_name = "VerticalDescriptor" - - show_in_read_only_mode = True - - # noinspection PyMethodParameters - @classproperty - def completion_mode(cls): # 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): - return XBlockCompletionMode.COMPLETABLE - - return XBlockCompletionMode.AGGREGATOR - display_name = String( display_name=_("Display Name"), help=_("The display name for this component."), @@ -154,257 +103,14 @@ def completion_mode(cls): # pylint: disable=no-self-argument 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 - default=[], - scope=Scope.user_state, - ) - # This cannot be called `show_reset_button`, because children blocks inherit this as a default value. - allow_resetting_children = 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."), - scope=Scope.settings, - default=False - ) @property def source_library_key(self): """ Convenience method to get the library ID as a LibraryLocator and not just a string. - - Supports only v1 libraries. """ return LibraryLocator.from_string(self.source_library_id) - @classmethod - def make_selection(cls, selected, children, max_count): - """ - 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 - - 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(list(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 - num_to_add = min(len(pool), num_to_add) - added_block_keys = set(rand.sample(list(pool), num_to_add)) - # We now have the correct n random children to show for this user. - selected_keys |= added_block_keys - - if any((invalid_block_keys, overlimit_block_keys, added_block_keys)): - selected = list(selected_keys) - random.shuffle(selected) - - return { - 'selected': selected, - 'invalid': invalid_block_keys, - 'overlimit': overlimit_block_keys, - 'added': added_block_keys, - } - - def _publish_event(self, event_name, result, **kwargs): - """ - Helper method to publish an event for analytics purposes - """ - event_data = { - "location": str(self.location), - "result": result, - "previous_count": getattr(self, "_last_event_result_count", len(self.selected)), - "max_count": self.max_count, - } - event_data.update(kwargs) - self.runtime.publish(self, f"edx.librarycontentblock.content.{event_name}", event_data) - 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): - """ - Helper method for publishing events when children blocks are - selected/updated for a user. This helper is also used by - the ContentLibraryTransformer. - - Arguments: - - block_keys - - A dict describing which events to publish (add or - remove), see `make_selection` above for format details. - - format_block_keys - - A function to convert block keys to the format expected - by publish_event. Must have the signature: - - [(block_type, block_id)] -> T - - Where T is a collection of block keys as accepted by - `publish_event`. - - publish_event - - Function that handles the actual publishing. Must have - the signature: - - <'removed'|'assigned'> -> result:T -> removed:T -> reason:str -> None - - Where T is a collection of block_keys as returned by - `format_block_keys`. - """ - if block_keys['invalid']: - # reason "invalid" means deleted from library or a different library is now being used. - publish_event( - "removed", - result=format_block_keys(block_keys['selected']), - removed=format_block_keys(block_keys['invalid']), - reason="invalid" - ) - - if block_keys['overlimit']: - publish_event( - "removed", - result=format_block_keys(block_keys['selected']), - removed=format_block_keys(block_keys['overlimit']), - reason="overlimit" - ) - - if block_keys['added']: - publish_event( - "assigned", - result=format_block_keys(block_keys['selected']), - added=format_block_keys(block_keys['added']) - ) - - def selected_children(self): - """ - 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) # pylint: disable=no-member - - # Publish events for analytics purposes: - lib_tools = self.get_tools() - format_block_keys = lambda keys: lib_tools.create_block_analytics_summary(self.location.course_key, keys) - self.publish_selected_children_events( - block_keys, - format_block_keys, - self._publish_event, - ) - - 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. - - return self.selected - - @XBlock.handler - def reset_selected_children(self, _, __): - """ - Resets the XBlock's state for a user. - - This resets the state of all `selected` children and then clears the `selected` field - so that the new blocks are randomly chosen for this user. - """ - if not self.allow_resetting_children: - return Response('"Resetting selected children" is not allowed for this XBlock', - status=status.HTTP_400_BAD_REQUEST) - - for block_type, block_id in self.selected_children(): - block = self.runtime.get_block(self.location.course_key.make_usage_key(block_type, block_id)) - if hasattr(block, 'reset_problem'): - block.reset_problem(None) - block.save() - - self.selected = [] - return Response(json.dumps(self.student_view({}).content)) - - def _get_selected_child_blocks(self): - """ - Generator returning XBlock instances of the children selected for the - current user. - """ - for block_type, block_id in self.selected_children(): - 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 - fragment = Fragment() - contents = [] - child_context = {} if not context else copy(context) - - for child in self._get_selected_child_blocks(): - if child is None: - # TODO: Fix the underlying issue in TNL-7424 - # This shouldn't be happening, but does for an as-of-now - # unknown reason. Until we address the underlying issue, - # let's at least log the error explicitly, ignore the - # exception, and prevent the page from resulting in a - # 500-response. - logger.error('Skipping display for child block that is None') - continue - - rendered_child = child.render(STUDENT_VIEW, 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_lms_template('vert_module.html', { - 'items': contents, - 'xblock_context': context, - 'show_bookmark_button': False, - 'watched_completable_blocks': set(), - 'completion_delay_ms': None, - 'reset_button': self.allow_resetting_children, - })) - - fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/library_content_reset.js')) - fragment.initialize_js('LibraryContentReset') - return fragment - def author_view(self, context): """ Renders the Studio views. @@ -448,23 +154,6 @@ def author_view(self, context): fragment.initialize_js('LibraryContentAuthorView') return fragment - def studio_view(self, _context): - """ - Return the studio view. - """ - fragment = Fragment( - self.runtime.service(self, 'mako').render_cms_template(self.mako_template, self.get_context()) - ) - add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor') - shim_xmodule_js(fragment, self.studio_js_module_name) - return fragment - - def get_child_blocks(self): - """ - Return only the subset of our children relevant to the current student. - """ - return list(self._get_selected_child_blocks()) - @property def non_editable_metadata_fields(self): non_editable_fields = super().non_editable_metadata_fields @@ -482,17 +171,6 @@ def get_tools(self, to_read_library_content: bool = False) -> 'LegacyLibraryTool return tools raise LibraryToolsUnavailable() - def get_user_id(self): - """ - Get the ID of the current user. - """ - user_service = self.runtime.service(self, 'user') - if user_service: - user_id = user_service.get_current_user().opt_attrs.get('edx-platform.user_id', None) - else: - user_id = None - return user_id - def _validate_sync_permissions(self): """ Raises PermissionDenied() if we can't confirm that user has write on this block and read on source library. @@ -623,6 +301,8 @@ 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. + + @@TODO put back what was here before. """ validation = super().validate() if not isinstance(validation, StudioValidation): @@ -711,8 +391,6 @@ 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. - - TODO: capa_type doesn't really need to trigger an upgrade once we've migrated to V2. """ 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)) @@ -723,57 +401,37 @@ def post_editor_saved(self, user, old_metadata, old_content): # pylint: disable # The validation area will display an error message, no need to do anything now. pass - def has_dynamic_children(self): - """ - Inform the runtime that our children vary per-user. - See get_child_blocks() above + def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) -> list[dict]: """ - return True - - def get_content_titles(self): + Implement format_block_keys_for_analytics using the modulestore-specific legacy library original-usage system. """ - Returns list of friendly titles for our selected children only; without - thi, all possible children's titles would be seen in the sequence bar in - the LMS. + def summarize_block(usage_key): + """ Basic information about the given block """ + orig_key, orig_version = self.runtime.modulestore.get_block_original_usage(usage_key) + return { + "usage_key": str(usage_key), + "original_usage_key": str(orig_key) if orig_key else None, + "original_usage_version": str(orig_version) if orig_version else None, + } - This overwrites the get_content_titles method included in x_module by default. - """ - titles = [] - for child in self.get_child_blocks(): - titles.extend(child.get_content_titles()) - return titles - - @classmethod - def definition_from_xml(cls, xml_object, system): - children = [] - - for child in xml_object.getchildren(): + result_json = [] + for block_key in block_keys: + key = self.context_key.make_usage_key(*block_key) + info = summarize_block(key) + info['descendants'] = [] try: - children.append(system.process_xml(etree.tostring(child)).scope_ids.usage_id) - except (XMLSyntaxError, AttributeError): - msg = ( - "Unable to load child when parsing Library Content Block. " - "This can happen when a comment is manually added to the course export." - ) - logger.error(msg) - if system.error_tracker is not None: - system.error_tracker(msg) - - definition = dict(xml_object.attrib.items()) - return definition, children - - def definition_to_xml(self, resource_fs): - """ Exports Library Content Block to XML """ - xml_object = etree.Element('library_content') - for child in self.get_children(): - self.runtime.add_block_as_child_node(child, xml_object) - # Set node attributes based on our fields. - for field_name, field in self.fields.items(): # pylint: disable=no-member - if field_name in ('children', 'parent', 'content'): - continue - if field.is_set_on(self): - xml_object.set(field_name, str(field.read_from(self))) - return xml_object + block = self.runtime.modulestore.get_item(key, depth=None) # Load the item and all descendants + children = list(getattr(block, "children", [])) + while children: + # @@TODO why is there branch info here now for us to clear?? + child_key = children.pop().replace(version=None, branch=None) + child = self.runtime.modulestore.get_item(child_key) + info['descendants'].append(summarize_block(child_key)) + children.extend(getattr(child, "children", [])) + except ItemNotFoundError: + pass # The block has been deleted + result_json.append(info) + return result_json class LegacyLibrarySummary: diff --git a/xmodule/library_tools.py b/xmodule/library_tools.py index 7f9e83a9373d..52a31c0608ea 100644 --- a/xmodule/library_tools.py +++ b/xmodule/library_tools.py @@ -58,44 +58,6 @@ def get_latest_library_version(self, library_id: str | LibraryLocator) -> str | assert library.location.library_key.version_guid is not None return str(library.location.library_key.version_guid) - def create_block_analytics_summary(self, course_key, block_keys): - """ - Given a CourseKey and 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. - """ - def summarize_block(usage_key): - """ Basic information about the given block """ - orig_key, orig_version = self.store.get_block_original_usage(usage_key) - return { - "usage_key": str(usage_key), - "original_usage_key": str(orig_key) if orig_key else None, - "original_usage_version": str(orig_version) if orig_version else None, - } - - result_json = [] - for block_key in block_keys: - key = course_key.make_usage_key(*block_key) - info = summarize_block(key) - info['descendants'] = [] - try: - block = self.store.get_item(key, depth=None) # Load the item and all descendants - children = list(getattr(block, "children", [])) - while children: - child_key = children.pop() - child = self.store.get_item(child_key) - info['descendants'].append(summarize_block(child_key)) - children.extend(getattr(child, "children", [])) - except ItemNotFoundError: - pass # The block has been deleted - result_json.append(info) - return result_json - def can_use_library_content(self, block): """ Determines whether a modulestore holding a course_id supports libraries. diff --git a/xmodule/tests/test_item_bank.py b/xmodule/tests/test_item_bank.py new file mode 100644 index 000000000000..54d4128f06d4 --- /dev/null +++ b/xmodule/tests/test_item_bank.py @@ -0,0 +1,497 @@ +""" +Unit tests for ItemBankBlock. + +@TODO - work in progress +""" + +from unittest.mock import MagicMock, Mock, patch + +import ddt +from fs.memoryfs import MemoryFS +from lxml import etree +from rest_framework import status +from web_fragments.fragment import Fragment +from xblock.runtime import Runtime as VanillaRuntime + +from openedx.core.djangolib.testing.utils import skip_unless_lms, skip_unless_cms +from xmodule.modulestore.tests.factories import CourseFactory +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 ..item_bank_block import ItemBankBlock +from .test_course_block import DummySystem as TestImportSystem + +dummy_render = lambda block, _: Fragment(block.data) # pylint: disable=invalid-name + + +class ItemBankTestBase(MixedSplitTestCase): + """ + Base class for tests of ItemBankBlock + """ + + def setUp(self): + super().setUp() + self.user_id = UserFactory().id + self.course = CourseFactory.create(modulestore=self.store) + self.chapter = self.make_block(category="chapter", parent_block=self.course, publish_item=True) + self.sequential = self.make_block(category="sequential", parent_block=self.chapter, publish_item=True) + self.vertical = self.make_block(category="vertical", parent_block=self.sequential, publish_item=True) + self.item_bank = self.make_block( + category="itembank", parent_block=self.vertical, max_count=1, display_name="My Item Bank", publish_item=True + ) + self.items = [ + self.make_block( + category="problem", parent_block=self.item_bank, display_name=f"My Item {i}", + data=f"

Hello world from problem {i}

", + publish_item=True, + ) + for i in range(4) + ] + self.publisher = Mock() # for tests that look at analytics + self._reload_item_bank() + + def _bind_course_block(self, block): + """ + Bind a block (part of self.course) so we can access student-specific data. + + @@TODO is this necessary? + """ + prepare_block_runtime(block.runtime, course_id=block.location.course_key) + + def get_block(descriptor): + """Mocks module_system get_block function""" + prepare_block_runtime(descriptor.runtime, course_id=block.location.course_key) + descriptor.runtime.get_block_for_descriptor = get_block + descriptor.bind_for_student(self.user_id) + return descriptor + + block.runtime.get_block_for_descriptor = get_block + + def _reload_item_bank(self): + """ + Reload self.item_bank. Do this if you want its `.children` list to be updated with what's in the db. + + HACK: These test cases don't persist student state, but some tests need `selected` to persist betweem item_bank + reloads. So, we just transfer it from the old item_bank object instance to the new one, as if it were persisted. + """ + selected = self.item_bank.selected + self.item_bank = self.store.get_item(self.item_bank.location) + self._bind_course_block(self.item_bank) + if selected: + self.item_bank.selected = selected + self.item_bank.runtime.publish = self.publisher + + +@skip_unless_cms +class TestItemBankForCms(ItemBankTestBase): + """ + Export and import tests for ItemBankBlock + """ + def test_xml_export_import_cycle(self): + """ + Test the export-import cycle. + """ + # Export self.item_bank to the virtual filesystem + export_fs = MemoryFS() + self.item_bank.runtime.export_fs = export_fs # pylint: disable=protected-access + node = etree.Element("unknown_root") + self.item_bank.add_xml_to_node(node) + + # Read back the itembank OLX + with export_fs.open('{dir}/{file_name}.xml'.format( + dir=self.item_bank.scope_ids.usage_id.block_type, + file_name=self.item_bank.scope_ids.usage_id.block_id + )) as f: + actual_olx_export = f.read() + + # And compare. + expected_olx_export = ( + '\n' + ' \n' + ' \n' + ' \n' + ' \n' + '\n' + ) + self.maxDiff = None + assert actual_olx_export == expected_olx_export + olx_element = etree.fromstring(actual_olx_export) + + # Re-import the OLX. + runtime = TestImportSystem(load_error_blocks=True, course_id=self.item_bank.location.course_key) + runtime.resources_fs = export_fs + imported_item_bank = ItemBankBlock.parse_xml(olx_element, runtime, None) + + # And make sure the result looks right. + self._verify_xblock_properties(imported_item_bank) + + def test_xml_import_with_comments(self): + """ + Test that XML comments within ItemBankBlock are ignored during the import. + """ + # Export self.item_bank to the virtual filesystem + export_fs = MemoryFS() + self.item_bank.runtime.export_fs = export_fs # pylint: disable=protected-access + node = etree.Element("unknown_root") + self.item_bank.add_xml_to_node(node) + + # Now, import the itembank using the same OLX as above, but with some XML comments in there. + # (Note: This will not work without exporting first, because it relies on the problem blocks OLX definitions + # being available on the filesystem) + olx_with_comments = ( + '\n' + '\n' + '\n' + ' \n' + ' \n' + ' \n' + ' \n' + '\n' + ) + olx_element = etree.fromstring(olx_with_comments) + + # Import the olx. + runtime = TestImportSystem(load_error_blocks=True, course_id=self.item_bank.location.course_key) + runtime.resources_fs = export_fs + imported_item_bank = ItemBankBlock.parse_xml(olx_element, runtime, None) + + # And make sure the result looks right. + self._verify_xblock_properties(imported_item_bank) + + def _verify_xblock_properties(self, imported_item_bank): + """ + Check the new XBlock has the same properties as the old one. + """ + assert imported_item_bank.display_name == self.item_bank.display_name + assert imported_item_bank.max_count == self.item_bank.max_count + assert len(imported_item_bank.children) == len(self.item_bank.children) + + def test_validation_of_matching_blocks(self): + """ + Test that the validation method of LibraryContent blocks can warn + the user about problems with other settings (max_count and capa_type). + + @@TODO + """ + # Ensure we're starting wtih clean validation + assert self.item_bank.validate() + + # Set max_count to higher value than exists in library + self.item_bank.max_count = 50 + result = self.item_bank.validate() + assert not result + self._assert_has_only_N_matching_problems(result, 4) + assert len(self.item_bank.selected_children()) == 4 + + # Add some capa problems so we can check problem type validation messages + self.item_bank.max_count = 1 + assert self.item_bank.validate() + assert len(self.item_bank.selected_children()) == 1 + + # -1 selects all blocks from the library. + self.item_bank.max_count = -1 + assert self.item_bank.validate() + assert len(self.item_bank.selected_children()) == len(self.item_bank.children) + + @patch( + 'xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.render', + VanillaRuntime.render, + ) + @patch('xmodule.capa_block.ProblemBlock.author_view', dummy_render, create=True) + @patch('xmodule.x_module.DescriptorSystem.applicable_aside_types', lambda self, block: []) + def test_preview_view(self): + """ Test preview view rendering """ + self._bind_course_block(self.item_bank) + rendered = self.item_bank.render(AUTHOR_VIEW, {'root_xblock': self.item_bank}) + assert '

Hello world from problem 0

' in rendered.content + assert '

Hello world from problem 1

' in rendered.content + assert '

Hello world from problem 2

' in rendered.content + assert '

Hello world from problem 3

' in rendered.content + + @patch( + 'xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.render', + VanillaRuntime.render, + ) + @patch('xmodule.capa_block.ProblemBlock.author_view', dummy_render, create=True) + @patch('xmodule.x_module.DescriptorSystem.applicable_aside_types', lambda self, block: []) + def test_author_view(self): + """ Test author view rendering """ + self._bind_course_block(self.item_bank) + rendered = self.item_bank.render(AUTHOR_VIEW, {}) + assert '' == rendered.content + # content should be empty + assert 'LibraryContentAuthorView' == rendered.js_init_fn + # but some js initialization should happen + + def _assert_has_only_N_matching_problems(self, result, n): + assert result.summary + assert StudioValidationMessage.WARNING == result.summary.type + assert f'only {n} have been selected' in result.summary.text + + +@skip_unless_lms +@ddt.ddt +class TestItemBankForLms(ItemBankTestBase): + """ + @@TODO + """ + + def _assert_event_was_published(self, event_type): + """ + Check that a LegacyLibraryContentBlock analytics event was published by self.item_bank. + """ + assert self.publisher.called + assert len(self.publisher.call_args[0]) == 3 # pylint:disable=unsubscriptable-object + _, event_name, event_data = self.publisher.call_args[0] # pylint:disable=unsubscriptable-object + assert event_name == f'edx.librarycontentblock.content.{event_type}' + assert event_data['location'] == str(self.item_bank.location) + return event_data + + def test_children_seen_by_a_user(self): + """ + Test that each student sees only one block as a child of the LibraryContent block. + """ + self._bind_course_block(self.item_bank) + # Make sure the runtime knows that the block's children vary per-user: + assert self.item_bank.has_dynamic_children() + + assert len(self.item_bank.children) == len(self.items) + + # Check how many children each user will see: + assert len(self.item_bank.get_child_blocks()) == 1 + # Check that get_content_titles() doesn't return titles for hidden/unused children + assert len(self.item_bank.get_content_titles()) == 1 + + def test_overlimit_blocks_chosen_randomly(self): + """ + Tests that blocks to remove from selected children are chosen + randomly when len(selected) > max_count. + """ + blocks_seen = set() + total_tries, max_tries = 0, 100 + + self._bind_course_block(self.item_bank) + + # Eventually, we should see every child block selected + while len(blocks_seen) != len(self.items): + self._change_count_and_reselect_children(len(self.items)) + # Now set the number of selections to 1 + selected = self._change_count_and_reselect_children(1) + blocks_seen.update(selected) + total_tries += 1 + if total_tries >= max_tries: + # The chance that this happens by accident is (4 * (3/4)^100) ~= 1/10^12 + assert False, "Max tries exceeded before seeing all blocks." + break + + def _change_count_and_reselect_children(self, count): + """ + Helper method that changes the max_count of self.item_bank, reselects + children, and asserts that the number of selected children equals the count provided. + """ + self.item_bank.max_count = count + selected = self.item_bank.get_child_blocks() + assert len(selected) == count + return selected + + @ddt.data( + # User resets selected children with reset button on content block + (True, 5), + # User resets selected children without reset button on content block + (False, 5), + # User resets selected children with reset button on content block when all library blocks should be selected. + (True, -1), + ) + @ddt.unpack + def test_reset_selected_children_capa_blocks(self, allow_resetting_children, max_count): + """ + Tests that the `reset_selected_children` method of a content block resets only + XBlocks that have a `reset_problem` attribute when `allow_resetting_children` is True + + This test block has 4 HTML XBlocks and 4 Problem XBlocks. Therefore, if we ensure + that the `reset_problem` has been called len(self.problem_types) times, then + it means that this is working correctly + """ + # Add a non-ProblemBlock just to make sure that this setting doesn't break with it. + self.make_block(category="html", parent_block=self.item_bank) + self._reload_item_bank() + + self.item_bank.allow_resetting_children = allow_resetting_children + self.item_bank.max_count = max_count + + # Mock the student view to return an empty dict to be returned as response + self.item_bank.student_view = MagicMock() + self.item_bank.student_view.return_value.content = {} + + with patch.object(ProblemBlock, 'reset_problem', return_value={'success': True}) as reset_problem: + response = self.item_bank.reset_selected_children(None, None) + + if allow_resetting_children: + self.item_bank.student_view.assert_called_once_with({}) + assert reset_problem.call_count == 4 # the # of problems in self.items + assert response.status_code == status.HTTP_200_OK + assert response.content_type == "text/html" + assert response.body == b"{}" + else: + reset_problem.assert_not_called() + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_assigned_event(self): + """ + Test the "assigned" event emitted when a student is assigned specific blocks. + """ + # In the beginning was the itembank and it assigned one child to the student: + child = self.item_bank.get_child_blocks()[0] + event_data = self._assert_event_was_published("assigned") + block_info = { + "usage_key": str(child.location), + } + assert event_data ==\ + {'location': str(self.item_bank.location), + 'added': [block_info], + 'result': [block_info], + 'previous_count': 0, 'max_count': 1} + self.publisher.reset_mock() + + # Now increase max_count so that one more child will be added: + self.item_bank.max_count = 2 + children = self.item_bank.get_child_blocks() + assert len(children) == 2 + child, new_child = children if children[0].location == child.location else reversed(children) + event_data = self._assert_event_was_published("assigned") + assert event_data['added'][0]['usage_key'] == str(new_child.location) + assert len(event_data['result']) == 2 + assert event_data['previous_count'] == 1 + assert event_data['max_count'] == 2 + + def test_removed_overlimit(self): + """ + Test the "removed" event emitted when we un-assign blocks previously assigned to a student. + We go from one blocks assigned to none because max_count has been decreased. + """ + # Decrease max_count to 1, causing the block to be overlimit: + self.item_bank.get_child_blocks() # This line is needed in the test environment or the change has no effect + self.publisher.reset_mock() # Clear the "assigned" event that was just published. + self.item_bank.max_count = 0 + + # Check that the event says that one block was removed, leaving no blocks left: + children = self.item_bank.get_child_blocks() + assert len(children) == 0 + event_data = self._assert_event_was_published("removed") + assert len(event_data['removed']) == 1 + assert event_data['result'] == [] + assert event_data['reason'] == 'overlimit' + + def test_removed_invalid(self): + """ + Test the "removed" event emitted when we un-assign blocks previously assigned to a student. + + In this test, we keep `.max_count==2`, but do a series of two removals from `.children`: + + * Starting condition: 4 children, 2 assigned: A [B] [C] D + * Delete the first assigned block (B) + * New condition: 3 children, 2 assigned: A _ [C] [D] + * Delete both assigned blocks (C, D) + * Final condition: 1 child, 1 asigned: [A] _ _ _ + + @@TODO - This is flaky!! Seems to be dependent on the random seed. Need to remove or delete. + """ + self.maxDiff = None + + # Start by assigning two blocks to the student: + self.item_bank.max_count = 2 + self.store.update_item(self.item_bank, self.user_id) + + # Sanity check + assert len(self.item_bank.children) == 4 + children_initial = [(child.block_type, child.block_id) for child in self.item_bank.children] + selected_initial: list[tuple[str, str]] = self.item_bank.selected_children() + assert len(selected_initial) == 2 + self.publisher.reset_mock() # Clear the "assigned" event that was just published. + + # Now make sure that one of the assigned blocks will have to be un-assigned. + # To cause an "invalid" event, we delete exactly one of the currently-assigned children: + to_keep: tuple[str, str] = selected_initial[0] + to_keep_usage_key = self.course.context_key.make_usage_key(*to_keep) + to_drop: tuple[str, str] = selected_initial[1] + to_drop_usage_key = self.course.context_key.make_usage_key(*to_drop) + self.store.delete_item(to_drop_usage_key, self.user_id) + self._reload_item_bank() + assert len(self.item_bank.children) == 3 # Sanity: We had 4 blocks, we deleted 1, should be 3 left. + + # Because there are 3 available children and max_count==2, when we reselect children for assignment, + # we should get 2. To maximize stability from the student's perspective, we expect that one of those children + # was the one that was previously assigned (to_keep). + selected_new = self.item_bank.selected_children() + assert len(selected_new) == 2 + assert to_keep in selected_new + assert to_drop not in selected_new + selected_new_usage_keys = [self.course.context_key.make_usage_key(*sel) for sel in selected_new] + + # and, obviously, the one block that was added to the selection should be one of the remaining 3 children. + (added_usage_key,) = set(selected_new_usage_keys) - set([to_keep_usage_key]) + added = (added_usage_key.block_type, added_usage_key.block_id) + assert added_usage_key in self.item_bank.children + + # Check that the event says that one block was removed and one was added + assert self.publisher.call_count == 2 + _, removed_event_name, removed_event_data = self.publisher.call_args_list[0][0] + assert removed_event_name == "edx.librarycontentblock.content.removed" + assert removed_event_data == { + "location": str(self.item_bank.usage_key), + "result": [{"usage_key": str(uk)} for uk in selected_new_usage_keys], + "previous_count": 2, + "max_count": 2, + "removed": [{"usage_key": str(to_drop_usage_key)}], + "reason": "invalid", + } + _, assigned_event_name, assigned_event_data = self.publisher.call_args_list[1][0] + assert assigned_event_name == "edx.librarycontentblock.content.assigned" + assert assigned_event_data == { + "location": str(self.item_bank.usage_key), + "result": [{"usage_key": str(uk)} for uk in selected_new_usage_keys], + "previous_count": 2, + "max_count": 2, + "added": [{"usage_key": str(added_usage_key)}], + } + self.publisher.reset_mock() # Clear these events + + # Now drop both of the selected blocks, so that only 1 remains (less than max_count). + for selected in selected_new: + self.store.delete_item(self.course.id.make_usage_key(*selected), self.user_id) + self._reload_item_bank() + assert len(self.item_bank.children) == 1 # Sanity: We had 3 blocks, we deleted 2, should be 1 left. + + # The remaining block should be one of the itembank's children, and it shouldn't be one of the ones that we had + # removed from the children. + (final,) = self.item_bank.selected_children() + final_usage_key = self.course.context_key.make_usage_key(*final) + assert final_usage_key in self.item_bank.children + assert final in children_initial + assert final not in {to_keep, to_drop, added} + + # Check that the event says that two blocks were removed and one added + assert self.publisher.call_count == 2 + _, removed_event_name, removed_event_data = self.publisher.call_args_list[0][0] + assert removed_event_name == "edx.librarycontentblock.content.removed" + assert removed_event_data == { + "location": str(self.item_bank.usage_key), + "result": [{"usage_key": str(final_usage_key)}], + "previous_count": 2, + "max_count": 2, + "removed": [{"usage_key": str(uk)} for uk in selected_new_usage_keys], + "reason": "invalid", + } + _, assigned_event_name, assigned_event_data = self.publisher.call_args_list[1][0] + assert assigned_event_name == "edx.librarycontentblock.content.assigned" + assert assigned_event_data == { + "location": str(self.item_bank.usage_key), + "result": [{"usage_key": str(final_usage_key)}], + "previous_count": 1, + "max_count": 2, + "added": [{"usage_key": str(final_usage_key)}], + } diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index 9914ef2d9098..092606142e1d 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -414,6 +414,7 @@ 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 LegacyLibraryContentBlock.source_library_version in non_editable_metadata_fields assert LegacyLibraryContentBlock.display_name not in non_editable_metadata_fields def test_overlimit_blocks_chosen_randomly(self):