-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: remove the branch/version while building BS #37866
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
fix: remove the branch/version while building BS #37866
Conversation
|
Thanks for the pull request, @marslanabdulrauf! 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. |
ormsbee
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.
Code looks fine, and I appreciate the comments. Can you please add a test case for this that would have failed with the old code?
|
@pdpinch, @marslanabdulrauf: Should this also be backported to Ulmo? |
|
Yes, it needs to be backported. |
30a7f72 to
dfa4497
Compare
dfa4497 to
5cd3aa4
Compare
|
Due to CI shenanigans, it took all day before these tests ran, and the openedx/cla check is hanging. Given that I know @marslanabdulrauf is covered under the MIT entity agreement (which out of paranoia I've just re-verified in SalesForce), and that is the only non-passing check, I'm going to use my admin bits to merge this in now. |
This commit updates the logic in the build_block_structure function to ensure that block locations are consistently normalized by removing branch and version information. This change addresses issues when creating a BlockStructure from modulestore using the published_only branch. Without this change, we end up comparing versioned keys to unversioned ones later on, which always yields False.
This commit updates the logic in the build_block_structure function to ensure that block locations are consistently normalized by removing branch and version information. This change addresses issues when creating a BlockStructure from modulestore using the published_only branch. Without this change, we end up comparing versioned keys to unversioned ones later on, which always yields False.
Related Ticket
https://github.com/mitodl/hq/issues/9703 (MIT Internal)
Description
Its a small ripple affect from #37335
This pull request updates the logic in the
build_block_structurefunction to ensure that block locations are consistently normalized by removing branch and version information. This change addresses issues when creating Block Structure frommodulestorewithinpublished_onlybranch.Useful information to include:
When creating BlockStructure within
published_onlybranch context the "xblock being changed" contains branch and version info causing comparison issues in next flows to remove inaccessible blocks from the outline.Testing instructions
Steps to reproduce
Testing