feat: adding event bus functionality #1848
Conversation
| ) | ||
| enterprise_enrollment.unenrolled_at = localized_utcnow() | ||
| enterprise_enrollment.save() | ||
| get_producer().send( |
There was a problem hiding this comment.
Does this need to be gated behind a feature_flag?
There was a problem hiding this comment.
I don't think it's a hard requirement but a best practice. It makes sense to have a feature flag in edx-platform or Credentials as there is likely going to be some time before the event bus is widely adopted by the Open edX Community. Relevant link to docs: https://openedx.atlassian.net/wiki/spaces/AC/pages/3508699151/How+to+start+using+the+Event+Bus#Producing
If there are any consumers of edx-enterprise outside of edx.org/2U, I would say it's a necessity. At the least, a temporary one is good in case we need to disable the functionality without a revert/code release.
| ) | ||
| enterprise_enrollment.unenrolled_at = localized_utcnow() | ||
| enterprise_enrollment.save() | ||
| get_producer().send( | ||
| signal=COURSE_UNENROLLMENT_COMPLETED, |
There was a problem hiding this comment.
this is a generic unenrollment signal but you only fire it for enterprise enrollments - i'd probably produce an event for all of these signals, not just the enterprise ones. i think this event is emitted somewhere upstream and that is where we should put that signal on the bus
| ) | ||
| enterprise_enrollment.unenrolled_at = localized_utcnow() | ||
| enterprise_enrollment.save() | ||
| get_producer().send( | ||
| signal=COURSE_UNENROLLMENT_COMPLETED, | ||
| topic='course-unenrollment-completed', |
There was a problem hiding this comment.
did you look into using a topic like 'course-enrollment-lifecycle'?
There was a problem hiding this comment.
FWIW, my understanding is the topics that are named with "{blah]-lifecycle" are when a topic handles multiple distinct events in a single topic.
The {environment}-learning-certificate-lifecycle events receive the "certificate awarded" and "certificate revoked" events.
@kiram15 -- One thing I will note, another best practice is that the name of the topic should contain the architecture subdomain the event belongs to. In this case, it is the learning subdomain. Whatever this topic name ends up being, it should include this subdomain (e.g. learning-{topic_name}).
| ) | ||
| enterprise_enrollment.unenrolled_at = localized_utcnow() | ||
| enterprise_enrollment.save() | ||
| get_producer().send( | ||
| signal=COURSE_UNENROLLMENT_COMPLETED, |
There was a problem hiding this comment.
Should we check if COUSE_ENROLLMENT_COMPLETED is not None before using it? It looks like it can potentially be None depending on if there was an ImportError.
There was a problem hiding this comment.
The reason I didn't put the check on it was later on in the code (line 410), we have a check that doesn't connect the receiver unless it is not None. I can put another check in if this is not sufficient though, but this seems to do it already.
if COURSE_UNENROLLMENT_COMPLETED is not None: COURSE_UNENROLLMENT_COMPLETED.connect(enterprise_unenrollment_receiver)
Adding event bus producer functionality to unenrollment event.
https://2u-internal.atlassian.net/browse/ENT-7560
Merge checklist:
requirements/*.txtfiles)base.inif needed in production but edx-platform doesn't install ittest-master.inif edx-platform pins it, with a matching versionmake upgrade && make requirementshave been run to regenerate requirementsmake statichas been run to update webpack bundling if any static content was updated./manage.py makemigrationshas been run./manage.py lms makemigrationsin the shell.Post merge:
(so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
make upgradein edx-platform will look for the latest version in PyPi.