-
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
feat: preliminary signal definition #9
Conversation
44e043b
to
91d3576
Compare
7ff3198
to
fd51d17
Compare
91d3576
to
b390065
Compare
45d7bfa
to
81e572f
Compare
aa94458
to
5eb16c4
Compare
81e572f
to
bb7d207
Compare
1744cbd
to
da85803
Compare
bb7d207
to
bad8b63
Compare
da85803
to
c1f9694
Compare
a76b654
to
57efe86
Compare
c1f9694
to
c3f8b8a
Compare
57efe86
to
bbaf18a
Compare
c3f8b8a
to
cdd8c32
Compare
bbaf18a
to
d57bc41
Compare
36d9a8c
to
665528a
Compare
00dd82c
to
53034b1
Compare
665528a
to
f330ee5
Compare
d2bf7ba
to
68a68fe
Compare
f330ee5
to
2e45593
Compare
68a68fe
to
7e611ed
Compare
0c0b2f5
to
b5882fa
Compare
7e611ed
to
890bfea
Compare
Hey @ormsbee, I hope you're doing good! I know you’re swamped, but we want to let you know how our advances are going. We've moved forward with the implementation of what was discussed in PR #4. It consists of three pull requests:
We're planning on taking the library with this PR upstream, after merging #7 and #8, as a way of exemplifying how events are used. So feel free to tag anyone interested! |
Thanks @mariajgrimaldi! @feanil: ^ |
openedx_events/learning/signals.py
Outdated
COURSE_ENROLLMENT_DEACTIVATED = OpenEdxPublicSignal( | ||
event_type="org.openedx.learning.course.enrollment.deactivated.v1", | ||
data={ | ||
"enrollment": CourseEnrollmentData, | ||
} | ||
) |
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.
This framing seems a bit model-centric (as opposed to event). vs. calling this something like UNENROLLED
.
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.
You're right, maybe we could use COURSE_UNENROLLMENT_COMPLETED
?
CERTIFICATE_CHANGED = OpenEdxPublicSignal( | ||
event_type="org.openedx.learning.certificate.changed.v1", | ||
data={ | ||
"certificate": CertificateData, | ||
} | ||
) |
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.
Do you need the state that it changed from?
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.
(genuine question–I really haven't thought about what folks are looking with this use case at all)
) | ||
from openedx_events.tooling import OpenEdxPublicSignal | ||
|
||
STUDENT_REGISTRATION_COMPLETED = OpenEdxPublicSignal( |
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.
Have you considered how these will be documented? Inline annotation documentation has the benefit of being close to the code, and would allow documentation to be added immediately along with the event definitions.
See toggles documentation as an example: https://edx.readthedocs.io/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html
Thanks.
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.
Excellent suggestion! We'll give it a go!
I'll tag you when it's ready!
openedx_events/learning/signals.py
Outdated
STUDENT_REGISTRATION_COMPLETED = OpenEdxPublicSignal( | ||
event_type="org.openedx.learning.student.registration.completed.v1", | ||
data={ | ||
"user": StudentData, |
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.
This naming mismatch is throwing me off a bit. Does it make more sense for the key to be student
instead of user
?
b5882fa
to
720838d
Compare
890bfea
to
7a8d549
Compare
720838d
to
b327358
Compare
7a8d549
to
222ed28
Compare
b327358
to
a23c0e2
Compare
222ed28
to
1e2211b
Compare
01158c5
to
9595a43
Compare
1e2211b
to
00d9946
Compare
00d9946
to
cddfda5
Compare
Description:
This PR adds the first 8 definitions of the Open edX events, they will be used in the Open edX platform.
JIRA: Link to JIRA ticket
Dependencies:
Depends on #8
Testing instructions:
With the event
STUDENT_REGISTRATION_COMPLETED
:In your Open edX devstack
make lms-shell
pip install git+https://github.com/eduNEXT/openedx-events.git@MJG/events_definition#egg=events_definition
We'll add soon an example using our plugin openedx-basic-hooks!
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.