feat: ItemBankBlock #35553
feat: ItemBankBlock #35553
Conversation
kdmccormick
force-pushed
the
kdmccormick/pool
branch
2 times, most recently
from
b403f34
to
d90f819
Compare
|
@kdmccormick Thanks! Here is the corresponding ticket for the UI: openedx/frontend-app-authoring#1385 |
kdmccormick
force-pushed
the
kdmccormick/pool
branch
from
d90f819
to
546fb28
Compare
kdmccormick
force-pushed
the
kdmccormick/pool
branch
from
199e0b1
to
6912ddb
Compare
| # TODO: Whittle down list of mixins if possible. | ||
| # https:/openedx/edx-platform/issues/35686 | ||
| MakoTemplateBlockBase, | ||
| XmlMixin, |
There was a problem hiding this comment.
It seems you can remove XmlMixin now. ItemBankBlock works fine without it AFAICT, although you may have to update some test cases that rely on its weird default behavior of writing the XML to a file in export_fs.
(edit: it may also affect import/export in some way, but I'm not sure)
Feel free to leave it in and get rid of it post-Sumac if that's more sensible.
| # https:/openedx/edx-platform/issues/35686 | ||
| MakoTemplateBlockBase, | ||
| XmlMixin, | ||
| XModuleToXBlockMixin, |
There was a problem hiding this comment.
This one also seems to be removable without causing any issues, at least for ItemBankBlock. Not sure if LegacyLibraryContentBlock needs it.
| # 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() |
There was a problem hiding this comment.
I don't like how this relies weird export behavior coming from XmlMixin, such that add_xml_to_node has a side-effect of writing definitions to the memory filesystem. According to the XmlMixin docstring, this behavior is "specifically for customtag ?".
I was going to say, I'd rather you remove XmlMixin and just use actual_olx_export = etree.tostring(node, encoding="unicode", pretty_print=True) to get the string version, instead of encoding these export nuances into the test case. If you need more advanced serialization (with children and static assets), call serialize_xblock_to_olx(self.item_bank) instead.
BUT I guess you're also relying on the fs-serialization of problem blocks and you're wanting to test code that's as similar as possible to the actual export-import cycle here? If so, that's fine. I just wish our import code wasn't so messy.
There was a problem hiding this comment.
I can try to simplify it.
BUT I guess you're also relying on the fs-serialization of problem blocks and you're wanting to test code that's as similar as possible to the actual export-import cycle here?
yes, but also test_library_content.py still exists and still has the near-identical test case that I cribbed this one from, so I think it's safe to use a cleaner version of the test case in test_item_bank.py
| ' <problem url_name="My_Item_3"/>\n' | ||
| '</itembank>\n' | ||
| ) | ||
| self.maxDiff = None |
There was a problem hiding this comment.
Nit: make this a static attribute of ItemBankTestBase perhaps?
| ' <problem url_name="My_Item_3"/>\n' | ||
| '</itembank>\n' | ||
| ) | ||
| olx_element = etree.fromstring(olx_with_comments) |
There was a problem hiding this comment.
As far as I know, this function will just skip over comments, so I'm not sure what this test is actually testing that's any different than if the comments didn't exist. We don't need to verify that etree can ignore comments when parsing XML.
| Test that the validation method of LibraryContent blocks can warn | ||
| the user about problems with other settings (max_count and capa_type). | ||
| """ | ||
| # Ensure we're starting wtih clean validation |
There was a problem hiding this comment.
| # Ensure we're starting wtih clean validation | |
| # Ensure we're starting with clean validation |
| 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" |
There was a problem hiding this comment.
I'm actually removing most of the validation messages in my UI PR, except the one for when max_count > num_children. Though it would be good to add validation that max_count isn't some invalid value like 0 or -2.
| 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). |
There was a problem hiding this comment.
| the user about problems with other settings (max_count and capa_type). | |
| the user about problems with settings (max_count). |
|
|
||
| def _assert_event_was_published(self, event_type): | ||
| """ | ||
| Check that a LegacyLibraryContentBlock analytics event was published by self.item_bank. |
There was a problem hiding this comment.
Nit: legacy doesn't appear in the event name so this is slightly confusing to me
"Check that an analytics event (called edx.librarycontentblock.content.____ for backwards compatibility) was published" perhaps?
| """ | ||
| rand = random.Random() | ||
|
|
||
| selected_keys = {tuple(k) for k in selected} # set of (block_type, block_id) tuples assigned to this student |
There was a problem hiding this comment.
[question]: Do you know what the call to tuple() here is for, if the thing being passed in is already a list of tuples? I'm hesitant to suggest any change to a method that has Obviously Seen Edge Cases, but it just seems weird. Maybe at some point someone just threw a list of lists and it threw things off because they wouldn't hash?
There was a problem hiding this comment.
Oh right, this 😛
It's because we assign lists-of-tuples to self.selected, but when you save the block and then load it back from ModuleStore, self.selected comes back as list-of-2-element-lists. So we need to handle both cases here by converting to tuples. I'll add a comment.
(Back in the last iteration of this project, I had straightened this out and made it type safe, but that never merged. I can't bite that off again right now, but I'd love to do it some time before the Sumac cut.)
| # Do we have enough blocks now? | ||
| num_to_add = max_count - len(selected_keys) | ||
|
|
||
| added_block_keys = None |
There was a problem hiding this comment.
[tiniest of optional nits]: I realize this was copied over from LibraryContentBlock, but the code uses empty sets to init the others like this, rather than None.
| "max_count": self.max_count, | ||
| } | ||
| event_data.update(kwargs) | ||
| self.runtime.publish(self, f"edx.librarycontentblock.content.{event_name}", event_data) |
There was a problem hiding this comment.
[question]: is this still the right event to emit given that it's going to be used in another block as well?
There was a problem hiding this comment.
Good question. I feel like Data WG would be the place to ask, but that won't happen between now and the cut.
I feel comfortable saying that LegacyLibraryContentBlocks should continue emitting edx.librarycontentblock.content.(assigned/removed)... events (for now, at least).
For the MVP, would would be your inclination for ItemBankBlocks? My inclination is either:
- Emit no events until we clear the schema with Data WG
- Emit
edx.itembankblock.content.(assigned/removed)events, but just withreasonandusage_keyfield. i.e., leave outoriginal_usage_key,original_usage_version, anddescendentsfor now.
(placeholder followup issue #35685)
| fragment = Fragment( | ||
| self.runtime.service(self, 'mako').render_cms_template(self.mako_template, self.get_context()) | ||
| ) | ||
| add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor') |
There was a problem hiding this comment.
[question]: Is it okay that we're using LibraryContentBlockEditor? Short term expediency?
There was a problem hiding this comment.
Yeah, short-term expediency to keep the children-display page working for both ItemBankBlock and LegacyLibraryContentBlock. There's actually nothing library-content-specific in that webpack bundle: https:/openedx/edx-platform/blob/master/webpack.builtinblocks.config.js#L41-L44
I'm happy to dive in later and see if it can be removed for one or both blocks, but for now, I'd rather leave it there.
| 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 |
There was a problem hiding this comment.
[question] Is this equivalent to using self.runtime.get_block, and an artifact of the fact that this used to be part of a class that had a reference to modulestore? Or is there a deeper difference? (No need to change it–this is really sensitive code and safe-equivalence is fine. I'm just curious to understand better.)
There was a problem hiding this comment.
I was going to ask the same thing, because the canonical way to iterate over children is just to use self.get_chidlren(). But I saw the "all descendants" - I think the intention here was something more optimized that loads many levels of decsendants at once? However, I don't really know why we care about that when 99% of problem banks will be one level deep.
In any case, this code was already here.
There was a problem hiding this comment.
an artifact of the fact that this used to be part of a class that had a reference to modulestore?
It's an artifact of that, plus me not remembering that self.runtime.get_block exists 😄
Using self.runtime.modulestore.get_item is already an imperfect substitute, as it spit out blocks with version information in their .children keys, hence the new .replace(version=None, branch=None) clause.
I'll try get_block, and see if it lets me remove the version-stripping.
Based on:
Description
A new XBlock that presents a random subset of its children.
The block does not care if its children are from V1 library, V2 library, or the course itself.
Shares the randomization logic with LegacyLibraryContentBlock. It is also fully backwards-compatible with LegacyLibraryContentBlock. So, once V1 libraries are migrated to V2 libraries, we eventually could point the
library_contententry point at ItemBankBlock.Supporting information
The unit editor view is identical to the LegacyLibraryContentBlock, except that the default title is "Problem Bank":
The editor (which Braden's team will replace) is currently just a subset of the settings on the LegacyLibraryContentBlock's editor:
Clicking "View" brings you to the detail view. Here's it empty; I haven't tested it with children yet:
Deadline / Status
We'd like this to be done and merged before the Sumac cutoff (23 Oct) if possible.
Current status: Partially working. Needs a new React-based editor for picking blocks from a V2 library. Also needs more unit tests.
Other information
...