-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: Add tags count for v2 library components #33979
feat: Add tags count for v2 library components #33979
Conversation
Thanks for the pull request, @yusuf-musleh! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
32c6980
to
3efde87
Compare
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.
LGTM 👍
- I tested this following the instructions from feat: View/Edit tags for Library Components openedx-unsupported/frontend-app-library-authoring#400
- I read through the code
-
I checked for accessibility issues - Includes documentation
Only a nit while replacing lib:
3efde87
to
9766937
Compare
49c6fa5
to
3dc1047
Compare
@bradenmacdonald This is ready for CC review. |
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.
Looks good to me. Just need to wait for that version bump and then we can merge this.
""" | ||
# Create a pattern to match the IDs of the library components, e.g. "lb:org:id*" | ||
library_key_pattern = str(library_key).replace("lib:", "lb:", 1) + "*" | ||
# TODO: add count_implicit=False when new tagging API version released |
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.
Don't you mean count_implicit=True
?
I opened a version bump PR here: openedx/openedx-learning#140
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.
Yup, I've update the PR with the new openedx-learning version and added the count_implicit=True
to it, working as expected now.
3dc1047
to
89a8be9
Compare
@yusuf-musleh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Includes content tags counts (including implicit) for Library V2 components in their metadata response.
Supporting information
Related Tickets:
Testing instructions
Follow the testing instructions on openedx-unsupported/frontend-app-library-authoring#400, specifically make sure the tags counts appear correctly.
Private-ref: FAL-3599