fix: address PR comments

xmodule/item_bank_block.py

@@ -26,7 +26,6 @@
 from xmodule.x_module import (
     ResourceTemplates,
     XModuleMixin,
-    XModuleToXBlockMixin,
     shim_xmodule_js,
     STUDENT_VIEW,
 )
@@ -43,7 +42,6 @@ class ItemBankMixin(
     # https://github.com/openedx/edx-platform/issues/35686
     MakoTemplateBlockBase,
     XmlMixin,
-    XModuleToXBlockMixin,
     ResourceTemplates,
     XModuleMixin,
     StudioEditableBlock,
@@ -101,10 +99,18 @@ 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
+            selected - list of (block_type, block_id) pairs assigned to this student
             children - children of this block
             max_count - number of components to display to each student
 
+        NOTE/TODO:
+            We like to treat `self.selected` as a list of 2-tuples, but when we load a block from persistence, all
+            tuples are quietly converted to lists, so `self.selected` actually ends up being a list of 2-element lists.
+            So, we need to carefully convert the items in `self.selected` to tuples whenever we work with it. As a
+            future refactoring, it would be much better to just accept that `self.selected` always has 2-element lists,
+            and then define some @properties that intentionally convert them to BlockKeys (named 2-tuples) plus type
+            annotations to enforce it all.
+
         Returns:
             A dict containing the following keys:
 
@@ -135,7 +141,7 @@ def make_selection(cls, selected, children, max_count):
         # Do we have enough blocks now?
         num_to_add = max_count - len(selected_keys)
 
-        added_block_keys = None
+        added_block_keys = set()
         if num_to_add > 0:
             # We need to select [more] blocks to display to this user:
             pool = valid_block_keys - selected_keys
@@ -159,13 +165,13 @@ def _publish_event(self, event_name, result, **kwargs):
         Helper method to publish an event for analytics purposes
         """
         event_data = {
-            "location": str(self.location),
+            "location": str(self.usage_key),
             "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.runtime.publish(self, f"{self.get_selected_event_prefix()}.{event_name}", event_data)
         self._last_event_result_count = len(result)  # pylint: disable=attribute-defined-outside-init
 
     @classmethod
@@ -280,7 +286,7 @@ def reset_selected_children(self, _, __):
                             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))
+            block = self.runtime.get_block(self.context_key.make_usage_key(block_type, block_id))
             if hasattr(block, 'reset_problem'):
                 block.reset_problem(None)
                 block.save()
@@ -314,7 +320,7 @@ def student_view(self, context):  # lint-amnesty, pylint: disable=missing-functi
             rendered_child = child.render(STUDENT_VIEW, child_context)
             fragment.add_fragment_resources(rendered_child)
             contents.append({
-                'id': str(child.location),
+                'id': str(child.usage_key),
                 'content': rendered_child.content,
             })
 
@@ -338,6 +344,9 @@ def studio_view(self, _context):
         fragment = Fragment(
             self.runtime.service(self, 'mako').render_cms_template(self.mako_template, self.get_context())
         )
+        # Note: The LibraryContentBlockEditor Webpack bundle just contains JS to help us render a vertical layout of
+        # children blocks. It's not clear if we need to include this bundle for ItemBankBlock, but it's working now.
+        # TODO: Try dropping this JS from ItemBankBlocks.
         add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor')
         shim_xmodule_js(fragment, self.studio_js_module_name)
         return fragment
@@ -414,6 +423,15 @@ def definition_to_xml(self, resource_fs):
                 xml_object.set(field_name, str(field.read_from(self)))
         return xml_object
 
+    @classmethod
+    def get_selected_event_prefix(cls) -> str:
+        """
+        Get a string prefix which will be prepended (plus a dot) to events raised when `self.selected` changes.
+
+        Example: if this returned `edx.myblock.content`, new selected children will raise `edx.myblock.content.assigned`.
+        """
+        raise NotImplementedError
+
 
 class ItemBankBlock(ItemBankMixin, XBlock):
     """
@@ -438,13 +456,16 @@ def validate(self):
             validation = StudioValidation.copy(validation)
         if not validation.empty:
             pass  # If there's already a validation error, leave it there.
-        elif not self.children:
+        elif self.max_count < -1 or self.max_count == 0:
             validation.set_summary(
                 StudioValidationMessage(
                     StudioValidationMessage.WARNING,
-                    (_('No problems have been selected.')),
+                    _(
+                        "The problem bank has been configured to show {count} problems. "
+                        "Please specify a positive number of problems, or specify -1 to show all problems."
+                    ).format(count=self.max_count),
                     action_class='edit-button',
-                    action_label=_("Select problems to randomize.")
+                    action_label=_("Edit the problem bank configuration."),
                 )
             )
         elif len(self.children) < self.max_count:
@@ -469,7 +490,7 @@ def author_view(self, context):
         """
         fragment = Fragment()
         root_xblock = context.get('root_xblock')
-        is_root = root_xblock and root_xblock.location == self.location
+        is_root = root_xblock and root_xblock.usage_key == self.usage_key
         # 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(
@@ -500,3 +521,13 @@ def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) ->
                 # "descendents": ...,
             } for block_key in block_keys
         ]
+
+    @classmethod
+    def get_selected_event_prefix(cls) -> str:
+        """
+        Prefix for events on `self.selected`.
+
+        TODO: Ensure that this is the prefix we want to stick with
+        https://github.com/openedx/edx-platform/issues/35685
+        """
+        return "edx.itembankblock.content"

xmodule/library_content_block.py

@@ -26,7 +26,7 @@
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.item_bank_block import ItemBankMixin
 from xmodule.validation import StudioValidation, StudioValidationMessage
-
+from xmodule.x_module import XModuleToXBlockMixin
 
 _ = lambda text: text
 
@@ -64,7 +64,7 @@ def __init__(self):
 
 @XBlock.wants('library_tools')
 @XBlock.wants('studio_user_permissions')  # Only available in CMS.
-class LegacyLibraryContentBlock(ItemBankMixin, XBlock):
+class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock):
     """
     An XBlock whose children are chosen dynamically from a legacy (v1) content library.
     Can be used to create randomized assessments among other things.
@@ -465,3 +465,13 @@ def display_number_with_default(self):
         Always returns the raw 'library' field from the key.
         """
         return self.location.library_key.library
+
+    @classmethod
+    def get_selected_event_prefix(cls) -> str:
+        """
+        Prefix for events on `self.selected`.
+
+        We use librarycontent rather than legacylibrarycontent for backwards compatibility (this wasn't always the
+        "legacy" library content block :)
+        """
+        return "edx.librarycontent.content"

xmodule/tests/test_item_bank.py

@@ -32,6 +32,7 @@ class ItemBankTestBase(MixedSplitTestCase):
     """
     Base class for tests of ItemBankBlock
     """
+    maxDiff = None  # We need all the diff we can get for some of these asserts.
 
     def setUp(self):
         super().setUp()
@@ -61,11 +62,11 @@ def _bind_course_block(self, block):
         (Not clear if this is necessary since XModules are all removed now. It's possible that this
          could be removed without breaking tests.)
         """
-        prepare_block_runtime(block.runtime, course_id=block.location.course_key)
+        prepare_block_runtime(block.runtime, course_id=block.context_key)
 
         def get_block(descriptor):
             """Mocks module_system get_block function"""
-            prepare_block_runtime(descriptor.runtime, course_id=block.location.course_key)
+            prepare_block_runtime(descriptor.runtime, course_id=block.context_key)
             descriptor.runtime.get_block_for_descriptor = get_block
             descriptor.bind_for_student(self.user_id)
             return descriptor
@@ -80,7 +81,7 @@ def _reload_item_bank(self):
         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.item_bank = self.store.get_item(self.item_bank.usage_key)
         self._bind_course_block(self.item_bank)
         if selected:
             self.item_bank.selected = selected
@@ -118,45 +119,11 @@ def test_xml_export_import_cycle(self):
             '  <problem url_name="My_Item_3"/>\n'
             '</itembank>\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 = (
-            '<!-- Comment -->\n'
-            '<itembank display_name="My Item Bank" max_count="1">\n'
-            '<!-- Comment -->\n'
-            '  <problem url_name="My_Item_0"/>\n'
-            '  <problem url_name="My_Item_1"/>\n'
-            '  <problem url_name="My_Item_2"/>\n'
-            '  <problem url_name="My_Item_3"/>\n'
-            '</itembank>\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 = TestImportSystem(load_error_blocks=True, course_id=self.item_bank.context_key)
         runtime.resources_fs = export_fs
         imported_item_bank = ItemBankBlock.parse_xml(olx_element, runtime, None)
 
@@ -174,9 +141,9 @@ def _verify_xblock_properties(self, imported_item_bank):
     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).
+        the user about problems with settings (max_count).
         """
-        # Ensure we're starting wtih clean validation
+        # Ensure we're starting with clean validation
         assert self.item_bank.validate()
 
         # Set max_count to higher value than exists in library
@@ -248,8 +215,8 @@ def _assert_event_was_published(self, event_type):
         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)
+        assert event_name == f'edx.itembankblock.content.{event_type}'
+        assert event_data['location'] == str(self.item_bank.usage_key)
         return event_data
 
     def test_children_seen_by_a_user(self):
@@ -349,10 +316,10 @@ def test_assigned_event(self):
         child = self.item_bank.get_child_blocks()[0]
         event_data = self._assert_event_was_published("assigned")
         block_info = {
-            "usage_key": str(child.location),
+            "usage_key": str(child.usage_key),
         }
         assert event_data ==\
-               {'location': str(self.item_bank.location),
+               {'location': str(self.item_bank.usage_key),
                 'added': [block_info],
                 'result': [block_info],
                 'previous_count': 0, 'max_count': 1}
@@ -362,9 +329,9 @@ def test_assigned_event(self):
         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)
+        child, new_child = children if children[0].usage_key == child.usage_key else reversed(children)
         event_data = self._assert_event_was_published("assigned")
-        assert event_data['added'][0]['usage_key'] == str(new_child.location)
+        assert event_data['added'][0]['usage_key'] == str(new_child.usage_key)
         assert len(event_data['result']) == 2
         assert event_data['previous_count'] == 1
         assert event_data['max_count'] == 2
@@ -401,8 +368,6 @@ def test_removed_invalid(self):
 
         @@TODO - This is flaky!! Seems to be dependent on the random seed. Need to fix. Should pin a specific seed, too.
         """
-        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)
@@ -441,7 +406,7 @@ def test_removed_invalid(self):
         # 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_name == "edx.itembankblock.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],
@@ -451,7 +416,7 @@ def test_removed_invalid(self):
             "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_name == "edx.itembankblock.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],
@@ -478,7 +443,7 @@ def test_removed_invalid(self):
         # 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_name == "edx.itembankblock.content.removed"
         assert removed_event_data == {
             "location": str(self.item_bank.usage_key),
             "result": [{"usage_key": str(final_usage_key)}],
@@ -488,7 +453,7 @@ def test_removed_invalid(self):
             "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_name == "edx.itembankblock.content.assigned"
         assert assigned_event_data == {
             "location": str(self.item_bank.usage_key),
             "result": [{"usage_key": str(final_usage_key)}],