feat: ItemBankBlock by kdmccormick · Pull Request #35553 · openedx/openedx-platform

kdmccormick · 2024-09-26T21:34:22Z

Based on:

feat!: Remove outdated Libraries Relaunch cruft #35644

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_content entry 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

...

bradenmacdonald · 2024-10-15T19:38:19Z

@kdmccormick Thanks! Here is the corresponding ticket for the UI: openedx/frontend-app-authoring#1385

bradenmacdonald

This is looking great, and working really well in my testing so far. Didn't run into any bugs.

bradenmacdonald · 2024-10-22T01:22:17Z

+    # TODO: Whittle down list of mixins if possible.
+    # https://github.com/openedx/edx-platform/issues/35686
+    MakoTemplateBlockBase,
+    XmlMixin,


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.

Yeah, I'll do this in a follow-up

bradenmacdonald · 2024-10-22T01:28:42Z

+        # 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()


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.

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

OK. Don't spend a ton of time on it though; more important we get this merged ASAP.

OK, I'd rather do this in a follow-up then

ormsbee · 2024-10-21T21:33:40Z

+        """
+        rand = random.Random()
+
+        selected_keys = {tuple(k) for k in selected}  # set of (block_type, block_id) tuples assigned to this student


[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?

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.)

Left a comment

ormsbee · 2024-10-21T21:48:09Z

+            "max_count": self.max_count,
+        }
+        event_data.update(kwargs)
+        self.runtime.publish(self, f"edx.librarycontentblock.content.{event_name}", event_data)


[question]: is this still the right event to emit given that it's going to be used in another block as well?

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 with reason and usage_key field. i.e., leave out original_usage_key, original_usage_version, and descendents for now.

(placeholder followup issue #35685)

I'm going with (2), LMK if that concerns you for Sumac

ormsbee · 2024-10-22T01:11:50Z

-            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


[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.)

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.

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.

This change didn't immediately work, so I'm going to punt on it for now

kdmccormick · 2024-10-22T16:33:13Z

Comments are addressed. I still have those 2 unit tests to fix-- I'll ping again when they're ready to go.

kdmccormick · 2024-10-22T19:14:16Z

+        ),
+    )
+    @ddt.unpack
+    def test_removed_invalid(self, to_select_initial, to_drop, to_select_new):


FYI @ormsbee , @bradenmacdonald -- I'm not proud of how long and convoluted this test case is, but I feel very confident that it thoroughly tests the selection & re-selection process, which has consistently shown itself to be the most complex aspect of this block. I'm willing to break it into smaller + more readable test cases in a follow-up, but I don't think it's important right now.

kdmccormick · 2024-10-22T19:14:45Z

Ready for re-review.

bradenmacdonald · 2024-10-22T19:18:24Z

-            'added': added_block_keys,
+            'invalid': sorted(invalid_block_keys),
+            'overlimit': sorted(overlimit_block_keys),
+            'added': sorted(added_block_keys),


Why are these now sorted? Is it to facilitate testing or fix a bug? I would assume it's better to have added reflect the order in which the blocks are added, but I don't think it matters.

To facilitate testing

bradenmacdonald

I have been testing this quite a bit along with my UI branch and it's been working well. Great work @kdmccormick.

kdmccormick · 2024-10-22T19:43:36Z

Thanks for the fast reviews all!

edx-pipeline-bot · 2024-10-22T21:28:20Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-10-22T21:45:10Z

2U Release Notice: This PR has been deployed to the edX production environment.

kdmccormick changed the title ~~feat: PoolBlock~~ feat: ProblemBankBlock Oct 10, 2024

kdmccormick changed the title ~~feat: ProblemBankBlock~~ feat: ItemBankBlock Oct 13, 2024

kdmccormick mentioned this pull request Oct 13, 2024

feat!: Remove outdated Libraries Relaunch cruft #35644

Merged

kdmccormick force-pushed the kdmccormick/pool branch 2 times, most recently from b403f34 to d90f819 Compare October 14, 2024 18:05

kdmccormick mentioned this pull request Oct 15, 2024

feat: Upstream Sync with Content Library Blocks #34925

Merged

bradenmacdonald mentioned this pull request Oct 15, 2024

UI to select components for the Problem Bank (MVP) openedx/frontend-app-authoring#1385

Closed

kdmccormick force-pushed the kdmccormick/pool branch from d90f819 to 546fb28 Compare October 18, 2024 01:12

bradenmacdonald mentioned this pull request Oct 20, 2024

"Bare Bones" UI for Problem Bank [FC-0062] #35679

Merged

4 tasks

kdmccormick mentioned this pull request Oct 21, 2024

ItemBankBlock <- Problem Bank UI kdmccormick/openedx-platform#17

Closed

kdmccormick force-pushed the kdmccormick/pool branch from 199e0b1 to 6912ddb Compare October 21, 2024 20:08

bradenmacdonald reviewed Oct 22, 2024

View reviewed changes

ormsbee reviewed Oct 22, 2024

View reviewed changes

kdmccormick added 4 commits October 22, 2024 12:32

feat: ItemBankBlock

cdef727

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

b51149b

docs: update the None-child mystery comment

f4cee43

fix: address PR comments

b28e11a

kdmccormick force-pushed the kdmccormick/pool branch from 934bf45 to b28e11a Compare October 22, 2024 16:32

fix: more PR review + comprehensive removed_invalid item testing

0f71b0e

kdmccormick marked this pull request as ready for review October 22, 2024 19:11

kdmccormick requested review from bradenmacdonald and ormsbee October 22, 2024 19:11

kdmccormick commented Oct 22, 2024

View reviewed changes

bradenmacdonald reviewed Oct 22, 2024

View reviewed changes

bradenmacdonald approved these changes Oct 22, 2024

View reviewed changes

kdmccormick merged commit 08c8e9d into openedx:master Oct 22, 2024

kdmccormick deleted the kdmccormick/pool branch October 22, 2024 19:40

kingoftech-v01 mentioned this pull request Apr 10, 2026

fix: withdrawn by author #38325

Closed

Uh oh!

Conversation

kdmccormick commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Deadline / Status

Other information

Uh oh!

bradenmacdonald commented Oct 15, 2024

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradenmacdonald Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick commented Oct 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick commented Oct 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

kdmccormick commented Oct 22, 2024

Uh oh!

edx-pipeline-bot commented Oct 22, 2024

Uh oh!

kdmccormick commented Sep 26, 2024 •

edited

Loading

bradenmacdonald Oct 22, 2024 •

edited

Loading

bradenmacdonald Oct 22, 2024 •

edited

Loading

kdmccormick Oct 22, 2024 •

edited

Loading

bradenmacdonald Oct 22, 2024 •

edited

Loading