-
Notifications
You must be signed in to change notification settings - Fork 21
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
[FC-0040] feat: Add CONTENT_OBJECT_TAGS_CHANGED
signal
#327
[FC-0040] feat: Add CONTENT_OBJECT_TAGS_CHANGED
signal
#327
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. |
0b0e2a5
to
ee63ddb
Compare
CONTENT_OBJECT_TAGGED
signalCONTENT_OBJECT_TAGGED
signal
""" | ||
Data about content object where tags changed |
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.
The docstring says where tags changed but the data attributes themselves are not associated with tagging, so could this be simply a data representation of the content?
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.
Agreed, I changed it to be more generic, representing the content object.
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.
I left a few comments for you to review. Thanks!
e4b9f69
to
758ba31
Compare
@mariajgrimaldi Thanks for the review! I've updated the PR addressing the comments. |
CONTENT_OBJECT_TAGGED
signalCONTENT_OBJECT_TAGS_CHANGED
signal
CONTENT_OBJECT_TAGS_CHANGED
signalCONTENT_OBJECT_TAGS_CHANGED
signal
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.
Good work @yusuf-musleh! 👍
- I tested this using the Testing Instructions from [FC-0040] feat: update search index when object tags change open-craft/edx-platform#647
- I read through the code
-
I checked for accessibility issues - Includes documentation
I left a few questions here: open-craft/edx-platform#647. I'll return once they've answered. Thanks! |
This signal is emitted when tags on a content object have changed.
758ba31
to
e967b0a
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. |
This signal is emitted when tags on a content object have changed.
Description: This PR adds a new
CONTENT_OBJECT_TAGGED
signal, which is emitted when a content object's tags have changed.ISSUE: openedx/modular-learning#197
Dependencies:
Testing instructions:
Follow instructions mentioned in open-craft/edx-platform#647
Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.
Private-ref: FAL-3691