-
Notifications
You must be signed in to change notification settings - Fork 14
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: replace signal handler with event producer config #14
base: main
Are you sure you want to change the base?
Conversation
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 tested this: Followed the testing instructions in platform PR and verified, signals are produced from the config without custom handlers.
- I read through the code
- I checked for accessibility issues
- Includes documentation
Thanks for the pull request, @navinkarkera! 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. |
Hi @navinkarkera! Would you mind resolving the branch conflicts that have popped up? Then I can get it reviewed. Thanks! |
799a6de
to
44721ca
Compare
Hi @sameenfatima78! Just following up on this :) |
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.
@sameenfatima78 Once openedx/openedx-events#272 is merged, I'll update this PR based on the decision in the ADR.
@navinkarkera now that #272 is merged, is this now ready-to-go? |
44721ca
to
0cee3b3
Compare
@mphilbrick211 @sameenfatima78 Thanks for your patience.
Done. Now it is ready. |
@navinkarkera - looks like there's still a failing test. Would you mind taking a look? |
c28e271
to
4f0048d
Compare
@mphilbrick211 @sameenfatima78 Sorry for the delay. Fixed the failing test and manually tested verification event in devstack. |
@sameenfatima78 Gentle reminder. |
Tagging @irfanuddinahmad here for review. |
Description: Replace the hard-coded signal handler with event bus producer config. See openedx/openedx-events#249 and openedx/edx-platform#32874 for more details.
JIRA:
Private-ref
: BB-7666Dependencies:
Merge deadline: List merge deadline (if any)
Testing instructions:
make lms-up
to get lms up and running.make lms-shell
.pip install -e /edx/src/xblock-skill-tagging/
(assuming this repo is cloned in<devstack-base-dir>/src/
)EVENT_BUS_PRODUCER_CONFIG
setting as per the configuration inREADME
inlms/envs/private.py
to pushXBLOCK_SKILL_VERIFIED
to event bus.XBLOCK_SKILL_VERIFIED
event.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.