feat: adding event bus functionality #1848
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |
|
|
||
| from logging import getLogger | ||
|
|
||
| from openedx_events.event_bus import get_producer | ||
|
|
||
| from django.db.models.signals import post_delete, post_save, pre_save | ||
| from django.dispatch import receiver | ||
|
|
||
|
|
@@ -349,16 +351,23 @@ def enterprise_unenrollment_receiver(sender, **kwargs): # pylint: disable=un | |
| """ | ||
| enrollment = kwargs.get('enrollment') | ||
| enterprise_enrollment = models.EnterpriseCourseEnrollment.objects.filter( | ||
| course_id=enrollment.course_id, | ||
| enterprise_customer_user__user_id=enrollment.enterprise_customer_user.user_id, | ||
| ).first() | ||
| if enterprise_enrollment: | ||
| logger.info( | ||
| f"Marking EnterpriseCourseEnrollment as unenrolled for user " | ||
| f"{enrollment.enterprise_customer_user.user_id} and course {enrollment.course_id}" | ||
| ) | ||
| 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 There was a problem hiding this comment. Should we check if 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.
|
||
| topic='course-unenrollment-completed', | ||
kiram15 marked this conversation as resolved.
Show resolved
Hide resolved
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 @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 |
||
| event_key_field='enrollment.course.course_key', | ||
| event_data={'enrollment': kwargs.get('enrollment')}, | ||
| event_metadata=kwargs.get('metadata') | ||
| ) | ||
|
|
||
|
|
||
| def create_enterprise_enrollment_receiver(sender, instance, **kwargs): # pylint: disable=unused-argument | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| amqp==5.1.1 | ||
| billiard==4.1.0 | ||
| celery==5.3.1 | ||
| click==8.1.7 | ||
| click-didyoumean==0.3.0 | ||
| click-repl==0.3.0 | ||
| kombu==5.3.1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
|
|
||
|
|
||
|
|
||
| # A central location for most common version constraints | ||
| # (across edx repos) for pip-installation. | ||
| # | ||
|
|
||
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.
Does this need to be gated behind a feature_flag?
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 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-enterpriseoutside 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.