diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index 24d81c9df85a..88a2c658af2e 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -114,10 +114,16 @@ def make_selection(cls, selected, children, max_count): 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 + 'selected' (list) of (block_type, block_id) tuples assigned to this student. + Order is RANDOM BUT ALSO SIGNIFICANT, because we want each student's order to remain stable as + long as their set of assigned children hasn't changed. + + 'invalid' (list) of dropped (block_type, block_id) tuples that are no longer valid + 'overlimit' (list) of dropped (block_type, block_id) tuples that were previously selected + 'added' (list) of newly added (block_type, block_id) tuples + + Order of invalid/overlimit/added is ALPHABETICAL, by type then id. This is arbitrary, but it makes the + events more consistent and easier to test. """ rand = random.Random() @@ -155,9 +161,9 @@ def make_selection(cls, selected, children, max_count): return { 'selected': selected, - 'invalid': invalid_block_keys, - 'overlimit': overlimit_block_keys, - 'added': added_block_keys, + 'invalid': sorted(invalid_block_keys), + 'overlimit': sorted(overlimit_block_keys), + 'added': sorted(added_block_keys), } def _publish_event(self, event_name, result, **kwargs): @@ -344,9 +350,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. + # TODO: It's not clear whether or why we need to add this JS bundle, but we're leaving it in for the Libraries + # Relaunch Beta because studio_view is currently working fine.. As future cleanup, try to remove it from + # the studio_view of ItemBankBlock and/or LegacyLibraryContentBlock. add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment @@ -428,7 +434,8 @@ 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`. + Example: if this returned `edx.myblock.content`, new selected children will + raise `edx.myblock.content.assigned`. """ raise NotImplementedError @@ -459,7 +466,7 @@ def validate(self): elif self.max_count < -1 or self.max_count == 0: validation.set_summary( StudioValidationMessage( - StudioValidationMessage.WARNING, + StudioValidationMessage.ERROR, _( "The problem bank has been configured to show {count} problems. " "Please specify a positive number of problems, or specify -1 to show all problems." diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index de430b4966ab..52e33108027c 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -430,6 +430,16 @@ def summarize_block(usage_key): result_json.append(info) return result_json + @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.librarycontentblock.content" + class LegacyLibrarySummary: """ @@ -465,13 +475,3 @@ 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" diff --git a/xmodule/tests/test_item_bank.py b/xmodule/tests/test_item_bank.py index 774cc6134477..8817f580e6b4 100644 --- a/xmodule/tests/test_item_bank.py +++ b/xmodule/tests/test_item_bank.py @@ -1,10 +1,8 @@ """ Unit tests for ItemBankBlock. - -@TODO - work in progress """ - from unittest.mock import MagicMock, Mock, patch +from random import Random import ddt from fs.memoryfs import MemoryFS @@ -138,32 +136,36 @@ def _verify_xblock_properties(self, imported_item_bank): 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): + def test_max_count_validation(self): """ - Test that the validation method of LibraryContent blocks can warn - the user about problems with settings (max_count). + Test that the validation method of ItemBankBlocks can warn the user about problems with settings (max_count). """ # Ensure we're starting with clean validation assert self.item_bank.validate() - # Set max_count to higher value than exists in library + # Ensure that setting the max_count too high (> than # of children) raises a validation warning. 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 + assert not (result := self.item_bank.validate()) + assert StudioValidationMessage.WARNING == result.summary.type + assert 'configured to show 50 problems, but only 4 have been selected' in result.summary.text - # Add some capa problems so we can check problem type validation messages - self.item_bank.max_count = 1 + # Now set max_count to valid value (<= than # of children), and ensure the validation error goes away. + self.item_bank.max_count = 3 + assert len(self.item_bank.selected_children()) == 3 assert self.item_bank.validate() - assert len(self.item_bank.selected_children()) == 1 - # -1 selects all blocks from the library. + # Ensure that setting max_count to 0 raises a validation error. + self.item_bank.max_count = 0 + assert len(self.item_bank.selected_children()) == 0 + assert not (result := self.item_bank.validate()) + assert StudioValidationMessage.ERROR == result.summary.type + assert 'configured to show 0 problems. Please specify' in result.summary.text + + # Finally, set max_count to -1, and ensure the validation error goes away. self.item_bank.max_count = -1 + assert len(self.item_bank.selected_children()) == 4 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', @@ -195,11 +197,6 @@ def test_author_view(self): 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 @@ -354,36 +351,62 @@ def test_removed_overlimit(self): assert event_data['result'] == [] assert event_data['reason'] == 'overlimit' - def test_removed_invalid(self): + @ddt.data( + ( + [("problem", "My_Item_1"), ("problem", "My_Item_2")], + ("problem", "My_Item_1"), + [("problem", "My_Item_2"), ("problem", "My_Item_3")], + ), + ( + [("problem", "My_Item_1"), ("problem", "My_Item_2")], + ("problem", "My_Item_2"), + [("problem", "My_Item_1"), ("problem", "My_Item_3")], + ), + ( + [("problem", "My_Item_3"), ("problem", "My_Item_0")], + ("problem", "My_Item_3"), + [("problem", "My_Item_2"), ("problem", "My_Item_3")], + ), + ( + [("problem", "My_Item_3"), ("problem", "My_Item_0")], + ("problem", "My_Item_0"), + [("problem", "My_Item_3"), ("problem", "My_Item_2")], + ), + ) + @ddt.unpack + def test_removed_invalid(self, to_select_initial, to_drop, to_select_new): """ 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`: + 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] _ _ _ + * Initial condition: 4 children, 2 assigned. 0 [1] [2] 3 + * First deletion: one of the assigned blocks, e.g. block 1. + * New condition: 3 children, 2 assigned. 0 _ [2] [3] + * Selecond deletion: the other two assigned blocks (2 and 3). + * Final condition: 1 child, 1 assigned. [0] _ _ _ - @@TODO - This is flaky!! Seems to be dependent on the random seed. Need to fix. Should pin a specific seed, too. + The grid on the right shows how the test should go for our first ddt case. """ + # pylint: disable=too-many-statements + # 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 + # Initial selection 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() + with patch.object(Random, "sample", _make_mock_sample(children_initial, to_select_initial)): + with patch.object(Random, "shuffle", _make_mock_shuffle(to_select_initial)): + selected_initial = 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,) = set(selected_initial) - set([to_drop]) 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() @@ -392,7 +415,12 @@ def test_removed_invalid(self): # 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() + remaining_selectable = set(children_initial) - {to_keep, to_drop} + to_add = set(to_select_new) & remaining_selectable + assert len(to_add) == 1 # sanity check + with patch.object(Random, "sample", _make_mock_sample(remaining_selectable, to_add)): + with patch.object(Random, "shuffle", _make_mock_shuffle(to_select_new)): + 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 @@ -449,7 +477,7 @@ def test_removed_invalid(self): "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], + "removed": [{"usage_key": str(uk)} for uk in sorted(selected_new_usage_keys)], "reason": "invalid", } _, assigned_event_name, assigned_event_data = self.publisher.call_args_list[1][0] @@ -461,3 +489,33 @@ def test_removed_invalid(self): "max_count": 2, "added": [{"usage_key": str(final_usage_key)}], } + + +def _make_mock_sample(expected_pool, mock_sample): + """ + A replacement for Random.sample that confirms that the pool and sample size are as expected. + """ + def sample(_self, pool, desired_sample_size): + """ + The mock implementation. + """ + assert set(pool) == set(expected_pool) + assert len(mock_sample) == desired_sample_size + return mock_sample + + return sample + + +def _make_mock_shuffle(mock_result): + """ + A replacement for Random.shuffle which confirms that the provided mock_result has the same set of items as + the selection we're shuffling. + """ + def shuffle(_self, selection): + """ + The mock implementation. + """ + assert set(selection) == set(mock_result) + return mock_result + + return shuffle