fix: almost done. just need to fix 2 unit tests

xmodule/item_bank_block.py

@@ -39,7 +39,8 @@
 @XBlock.needs('mako')
 @XBlock.wants('user')
 class ItemBankMixin(
-    # @@TODO do we really need all these mixins?
+    # TODO: Whittle down list of mixins if possible.
+    # https://github.com/openedx/edx-platform/issues/35686
     MakoTemplateBlockBase,
     XmlMixin,
     XModuleToXBlockMixin,
@@ -263,9 +264,6 @@ def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) ->
         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
 
@@ -385,7 +383,7 @@ def get_content_titles(self):
     @classmethod
     def definition_from_xml(cls, xml_object, system):
         """
-        @@TODO docstring
+        Parse an itembank.
         """
         children = []
 
@@ -435,8 +433,6 @@ class ItemBankBlock(ItemBankMixin, XBlock):
     def validate(self):
         """
         Validates the state of this ItemBankBlock Instance.
-
-        @@TODO implement
         """
         validation = super().validate()
         if not isinstance(validation, StudioValidation):
@@ -494,7 +490,14 @@ def author_view(self, context):
     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]
+        return [
+            {
+                "usage_key": str(self.context_key.make_usage_key(*block_key)),
+                # TODO: Need to implement these fields. Will probably need to make `UpstreamMixin` available in the
+                #       LMS. See https://github.com/openedx/edx-platform/issues/35685.
+                # "original_usage_key": ...,
+                # "original_usage_version": ...,
+                # "descendents": ...,
+            } for block_key in block_keys
+        ]

xmodule/library_content_block.py

@@ -301,8 +301,6 @@ 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):
@@ -410,7 +408,7 @@ def summarize_block(usage_key):
             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_key": str(orig_key.replace(version=None, branch=None)) if orig_key else None,
                 "original_usage_version": str(orig_version) if orig_version else None,
             }
 
@@ -423,7 +421,6 @@ def summarize_block(usage_key):
                 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))

xmodule/tests/test_item_bank.py

@@ -58,7 +58,8 @@ def _bind_course_block(self, block):
         """
         Bind a block (part of self.course) so we can access student-specific data.
 
-        @@TODO is this necessary?
+        (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)
 
@@ -89,7 +90,7 @@ def _reload_item_bank(self):
 @skip_unless_cms
 class TestItemBankForCms(ItemBankTestBase):
     """
-    Export and import tests for ItemBankBlock
+    Test Studio ItemBank behaviors -- export/import, validation, author-facing views.
     """
     def test_xml_export_import_cycle(self):
         """
@@ -174,8 +175,6 @@ 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()
@@ -197,6 +196,8 @@ def test_validation_of_matching_blocks(self):
         assert self.item_bank.validate()
         assert len(self.item_bank.selected_children()) == len(self.item_bank.children)
 
+        assert "@@TODO make more assertions about the validation messages"
+
     @patch(
         'xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.render',
         VanillaRuntime.render,
@@ -237,7 +238,7 @@ def _assert_has_only_N_matching_problems(self, result, n):
 @ddt.ddt
 class TestItemBankForLms(ItemBankTestBase):
     """
-    @@TODO
+    Test LMS ItemBank features: selection, analytics, resetting problems.
     """
 
     def _assert_event_was_published(self, event_type):
@@ -398,7 +399,7 @@ def test_removed_invalid(self):
         * 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.
+        @@TODO - This is flaky!! Seems to be dependent on the random seed. Need to fix. Should pin a specific seed, too.
         """
         self.maxDiff = None