Skip to content
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

Closed
wants to merge 82 commits into from

Conversation

connorhaugh
Copy link
Contributor

@connorhaugh connorhaugh commented Aug 18, 2023

An updated feature branch containing:
https://github.com/openedx/edx-platform/pull/31319/files
#30895
#30801
#30800

TODO:

  • Quality Checks
  • No “configure” button
  • Library Content Selection page loads
  • Libraries can be configured to only show V2 libraries for both Randomized Content and library blocks.

dyudyunov and others added 30 commits July 27, 2022 16:39
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.
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.
@connorhaugh connorhaugh changed the title Feat working library source xblock feat: working library source xblock Aug 28, 2023
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a 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)
Copy link
Contributor

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

cms/static/js/views/pages/container.js Show resolved Hide resolved
cms/static/js/views/pages/container.js Show resolved Hide resolved
cms/static/js/views/pages/container.js Show resolved Hide resolved
cms/static/js/views/pages/container.js Show resolved Hide resolved
xmodule/library_tools.py Show resolved Hide resolved
xmodule/tasks.py Show resolved Hide resolved
xmodule/tasks.py Show resolved Hide resolved
xmodule/tasks.py Show resolved Hide resolved
xmodule/tasks.py Outdated Show resolved Hide resolved
@connorhaugh
Copy link
Contributor Author

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

Copy link
Member

@kdmccormick kdmccormick left a 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.

lms/envs/common.py Show resolved Hide resolved
cms/static/js/views/pages/container.js Show resolved Hide resolved
@connorhaugh connorhaugh marked this pull request as ready for review September 12, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants