-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: apply library_content transformer to all ItemBankMixin xblocks #37830
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?
fix: apply library_content transformer to all ItemBankMixin xblocks #37830
Conversation
|
Thanks for the pull request, @MaferMazu! 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. DetailsWhere 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. |
f3c61ae to
2682a9a
Compare
95561fb to
8090f8a
Compare
bradenmacdonald
left a comment
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.
Thanks for this PR! It seems like an important bug to fix, and I appreciate the clear description and diagnosis. I did some testing just now and can confirm the bugs you're seeing. I haven't tried this fix out yet, but it seems right to me.
| # Fallback for old cache that doesn't have the 'is_item_bank_block' field | ||
| if is_item_bank is None: | ||
| block_class = _get_block_class(block_key.block_type) | ||
| is_item_bank = block_class and issubclass(block_class, ItemBankMixin) |
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.
Is this just temporary code? Do we need it at all? Or is this why READ_VERSION is left at 1? If so, for how long do we need to keep this code in here? (Same question below)
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.
Yes, it is temporary code. I added to keep read_version at 1 (so we can use the old cache if available). If a course has an old cache, that section will execute. I added it to avoid changing the read_version to 2, which, in my understanding, forces the platform to discard all the xblock structure cache (and can cause issues in large instances).
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.
About how long: I found that the cache is regenerated by default every 24h, so if someone migrates to a version with this change in ~1 or 2 weeks, it should be safe to remove the fallback code. In the case of openedx I was thinking of 1 release. I added that fallback code to avoid discarding the entire xblock structure cache at once.
References:
- Cache time out: https://github.com/openedx/openedx-platform/blob/master/openedx/core/djangoapps/content/block_structure/config/models.py#L17
- More about read/write version and transformers: https://github.com/openedx/openedx-platform/blob/master/openedx/core/djangoapps/content/block_structure/transformer.py#L46
|
|
||
| # Fallback for old cache that doesn't have the 'is_item_bank_block' field | ||
| if is_item_bank is None: | ||
| block_class = _get_block_class(block_key.block_type) | ||
| is_item_bank = block_class and issubclass(block_class, ItemBankMixin) |
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.
Same question about this [temporary?] code.
8090f8a to
94c5cf5
Compare
f824b52 to
68a362d
Compare
|
@bradenmacdonald, thanks for reviewing this and for your patience. I just answered your feedback. I'm attentive to further feedback. ✨ |
Description
This PR adds logic to the library_content transformer to support all xblocks that inherit from ItemBankMixin.
Supporting information
Specific case (The problem)
When creating a Problem Bank component, the structure of the xblock doesn't match (the user can solve 3 problems of 5), in the grade the system evaluates 5 problems when only 3 are solvable for the user.
Inside
The transformer didn't remove the non-selected children from the problem bank (randomized content from a content library v2 - itembankblock in the code), so the structure shows inconsistent information (more questions/grades than the user is available to see/solve)
Solution
Adding the logic to analyze the block_class and check whether it inherits from itembankmixin. I apply the same logic as the legacy library content to all xblocks with randomized content, without removing support for the legacy libraries.
Testing instructions
Pre-requisites:
Test:
LegacyLibraries
Things to verify in LMS (with the live version or a student account):
ProblemBank
Things to verify in LMS (with the live version or a student account):
The same from the LegacyLibraries
Another Xblock (optional)
Things to verify in LMS (with the live version or a student account):
The same from the LegacyLibraries
Screenshots
Base

Problem Bank Configuration (to show 1 of 4 problems)
Before

Problem Bank in LMS (problem: in the sidebar bar, it shows we had four problems available when we have 1)
After


Problem Bank in LMS (It shows we have 1 question) ✅
Progress in LMS (In the grade details, we have the correct information) ✅