-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: working library source xblock #33057
Conversation
YT: https://youtrack.raccoongang.com/issue/EDX_BLND_CLI-87 - V2 libraries are available for selection in the Random Block edit modal; - selected V2 library blocks are copied to the modulestore and saved as children of the Random Block; - V2 library version validation works the same as for the V1 libraries (with possibility to update block with the latest version); - filtering by problem type can't be done for V2 the same as for V1 because the v2 library problems are not divided by types; - unit tests added/updated.
…e top default level
YT: https://youtrack.raccoongang.com/issue/EDX_BLND_CLI-87 - V2 libraries are available for selection in the Random Block edit modal; - selected V2 library blocks are copied to the modulestore and saved as children of the Random Block; - V2 library version validation works the same as for the V1 libraries (with possibility to update block with the latest version); - filtering by problem type can't be done for V2 the same as for V1 because the v2 library problems are not divided by types; - unit tests added/updated.
…e top default level
…edx-platform into feat--Working-Library-Source-Xblock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ended up being more nit-picky than I intended, but I guess that means I didn't see any major issues :) Looking good, and feel free to ignore or push back on any of this.
self.assertEqual(self.problem_in_course.location, self.lc_block.children[0]) | ||
self.assertEqual(self.problem_in_course.display_name, self.original_display_name) | ||
# and the block has changed too. | ||
self.assertEqual(len(self.lc_block.children), 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests is no longer testing what it says in the docstring. I think it's important to preserve a test for that ("if a library content block is pinned to some library version and duplicated, the new block is pinned to the same version, and the children are NOT updated to the newest version")
@bradenmacdonald thanks for the precise feedback I'll take all of this into consideration. A good amount of this work is assembling pieces of other's past work, but hopefully your approach will allow me to put in quick fixes for many of these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, just a few questions I had while skimming the code this morning.
…edx-platform into feat--Working-Library-Source-Xblock
An updated feature branch containing:
https://github.com/openedx/edx-platform/pull/31319/files
#30895
#30801
#30800
TODO: