diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index e7b1e3bdb0b..7ea9b8bdd4a 100644 --- a/xmodule/item_bank_block.py +++ b/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 + ] diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index c2d3aef623d..1adcaae9d30 100644 --- a/xmodule/library_content_block.py +++ b/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)) diff --git a/xmodule/tests/test_item_bank.py b/xmodule/tests/test_item_bank.py index 54d4128f06d..33a3eef39c1 100644 --- a/xmodule/tests/test_item_bank.py +++ b/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