-
Notifications
You must be signed in to change notification settings - Fork 11
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: Update ObjectTagView and ObjectTagCountsView to accept complex objectIds #246
fix: Update ObjectTagView and ObjectTagCountsView to accept complex objectIds #246
Conversation
Thanks for the pull request, @marslanabdulrauf! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by 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. |
Hi @marslanabdulrauf, thank you for this contribution! Please let me know if you have any questions re: submitting a CLA form. Thanks! |
@mphilbrick211 -- @marslanabdulrauf is working for MIT on this PR and he should be covered by the MIT entity agreement. I've just added him to the MITx Staff and GitHub details google sheet. |
2ad02e4
to
8d7d34d
Compare
8d7d34d
to
facb194
Compare
4fbe685
to
a215c0a
Compare
Hi @marslanabdulrauf can you add tests for both scenarios? |
yeah sure @Ian2012 |
@ormsbee Do you want to take a look at this one? |
@Ian2012: Please don't block on my review. But if you're asking me to be another reviewer, I can do that–but I probably won't get to it until Monday. |
@marslanabdulrauf quality tests are failing. I can merge once CI passes. |
@marslanabdulrauf Once it's bumped, it's good to go. |
@Ian2012 I am unclear, how much time this might take ? Current status at the time of writing:
|
Sorry, I wasn't clear. I want you to bump the version to merge and release it. I think we can merge it to master and upgrade the version, the current changes on |
ObjectTagView and ObjectTagCountsView are updated to accept complex ObjectIds
… object_ids ObjectTagView and ObjectTagCountView tests updated to work against complext object_ids as well
6569307
to
9de4145
Compare
Hi @Ian2012,
Would you be open to rolling out a 0.16.2 release with the backported change? If not, could we explore how best to align with SUMAC release with the latest openedx-learning? Let me know your thoughts or if you'd like us to assist further. Thanks again! |
We already performed this change on edx-platform: openedx/edx-platform#35788, and openedx-learning it's already on v0.18.0 on edx-platform: https://github.com/openedx/edx-platform/blob/open-release/sumac.master/requirements/edx/base.txt#L828 |
Hi @Ian2012, I checked and noticed that Could you confirm how we should proceed to ensure compatibility? |
Please create a PR with the version bump of openedx-learning on edx-platform, then you can post it to the |
Current:
The current approach is to save the id of the content through a generic string, so that the tag can be linked to any type of content, no matter what type of ID have. For example, with this approach we can link standalone blocks and library blocks, both on Denver (v3) and Blockstore Content Libraries (v2).
Issue:
With complex object_id e.g;
course-v1:MIT+lt101.1+101.1
which is a acceptable format, django resolver is throwing 404What this PR does?
This PR is adding a regex for
object_id
lookup field value inObjectTagView
andObjectTagCountsView
to make it accept special characters specially those which are common in openedx course creationBefore:
After: