-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: follow migrated legacy library content block #37405
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
base: master
Are you sure you want to change the base?
feat: follow migrated legacy library content block #37405
Conversation
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
6dbd6db
to
8c646c1
Compare
7ad846c
to
06de64c
Compare
xmodule/library_content_block.py
Outdated
if self.is_source_lib_migrated_to_v2 and not self.is_migrated_to_v2: | ||
# If the source library is migrated but this block still depends on legacy library | ||
# Migrate the block by setting upstream field to all children blocks | ||
self._v2_update_children_upstream_version() |
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.
@navinkarkera I'm not sure this is the best approach. The point of this method is to render the block, not to modify it--sticking the migration here is surprising and auto-magical. It will also update the children when the block is rendered in LMS, which would either fail or be a no-op, and would be confusing either way. In general, we should not be modifying content except in response to the action of a Studio user.
I had tried to outline an idea to follow library_content child references without a child migration process, but since writing that, I've also come to dislike the idea, because it would break the simplifying assumption that we can just check self.upstream
on any block to see its upstream link. So, I like that your PR uses a migration child step, but I think we need some explicit way to trigger it.
Here's an idea:
- When a legacy library_content block has (a) unmigrated chidlren and (b) a migrated source library, it gives the users the option to "sync" the latest content, regardless of whether are new changes in the target library.
- Syncing, in this case, will migrate the children.
It's a slight product behavior change. In particular, every legacy library content block would have a "sync available" as soon as an operator migrates its source library. But, I think it would be worth the simplified behavior information architecture. What do you think?
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.
@kdmccormick This is not the final version, locally I have something like this:
Sorry for the confusion.
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.
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.
Cool, thanks. I think this is a good approach. I'll ask precisely what the UI should be at the sync-up tomorrow.
82767cb
to
c2094f2
Compare
…lity Newly added blocks from library in children view page of item bank block and migrated library content block were not displayed automatically.
c2094f2
to
d993b55
Compare
Description
Allows authors to migrate legacy library content block in course to library v2 if the source library was migrated. The block works with the new library and has the same functionality as the new ItemBankBlock.
This PR also fixes some issues with Problem Bank like
Add components
button not working in the preview page and iframe postMessage failure after syncing of component in the preview page.screencast.mp4
Requirements as per The LegacyLibraryContentBlock OLX must work forever.
Sync behavior
Useful information to include:
Supporting information
Private-ref
: https://tasks.opencraft.com/browse/FAL-4251Testing instructions
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.