Skip to content
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.

Sign up for GitHub

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 #18

Merged
merged 1 commit into from
Aug 18, 2021
Merged

feat: preliminary signal definition #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ Change Log
Unreleased
~~~~~~~~~~

Added
_____
* Preliminary Open edX events definitions.

[0.3.0] - 2021-08-18
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Added
Expand Down
110 changes: 110 additions & 0 deletions openedx_events/learning/signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,113 @@
They also must comply with the payload definition specified in
docs/decisions/0003-events-payload.rst
"""

from openedx_events.learning.data import CertificateData, CohortData, CourseEnrollmentData, UserData
from openedx_events.tooling import OpenEdxPublicSignal

# .. event_type: org.openedx.learning.student.registration.completed.v1
# .. event_name: STUDENT_REGISTRATION_COMPLETED
# .. event_description: emitted when the user registration process in the LMS is completed.
# .. event_data: UserData
STUDENT_REGISTRATION_COMPLETED = OpenEdxPublicSignal(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @robrap! I hope you're doing great!

You mentioned inline code annotations here. And we followed your suggestion in this PR (is the same one as in the comment, but I deleted #9 by accident)

This is our take on it. We also thought about taking this to the edx code-annotations repo. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @mariajgrimaldi. Great start. Thanks. Some quick initial thoughts:

  1. See this how-to on code annotations and more: https://code-annotations.readthedocs.io/en/latest/contrib/how_to/add_new_annotations_and_extracted_docs.html
  2. I'd err on dropping annotations that aren't high value. Others may be in a better position to help make these decisions, but some food for thought:
    a. event_version: if this is built into the type, do you need the redundant data that could get out of sync?
    b. event_implementation: How many are there? If just one, do we need this yet? Or, do we want to detail what the implementation types would be? (Note: for toggles, it is a statically defined list.) Defining the annotation configuration file in a separate PR might help hammer out that discussion. If we need to keep this annotation, I'd recommend OpenEdxPublicSignal (or DjangoSignal), but probably not both. Ultimately, there will be a how-to for writing these annotations that will explain more details.
    c. event_creation_date: We had this for toggles (not settings) to help with deprecation/removal of temporary toggles? Do you see a similar important need to have the date for this, or can this be dropped?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @robrap, thanks for your quick response.

2a. You're right. I removed it! Thanks.
2b. For now, it's just one, so I'll follow your suggestion. We can review this later if necessary.
2c. I thought it could be useful for events versioning -eg. when moving from v1 to v2-. Don't you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates.

For 2c, I do not know yet. I am unclear on how versioning will evolve and how this metadata will be used. For now, since I think it is 100% redundant with the suffix of the event_type, and you'd probably need linting to ensure that the redundant data matches, I think it could be left off for now. I imagine the Sphinx plugin (if you go that route) could also pull the version off the event_type if you want it to be reported separately.

Also, feel free to let me know if there are known cases where these will not match.

Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @robrap! Hope you're doing great! Thanks again for the feedback.

BTW, we thought about adding ..event_status. More about the status here

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robrap, we are picking this up after our summer break, but we are thinking now with @mariajgrimaldi that this discussion requires its own PR since the the feedback about the definition itself seems to be taken in and it is resolved.

We would like to merge this very soon and focus on the PR that brings this into the platform. At the same time we will open a new PR about the status field in the inline documentation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felipemontoya: I'm not clear on your proposal for handling this PR. I'd recommend that you land the docs as written, since it helps document the events, and then iterate and improve if you want to update/solidify the annotations in a separate PR (or PRs). Is that what you are thinking?

@mariajgrimaldi: .. event_status seems reasonable, assuming the ADR is accepted. I imagine that "removed" and "replaced" wouldn't be needed in the code annotations definitions, because these events and their docs simply wouldn't exist any more? This question doesn't need to be resolved on this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Looks like we are leaving in these initial docs. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @robrap. Yeah, the plan is to leave the docs as they were after your first round of reviews and manage only the lines with "event_status" and the ADR acceptance in the new PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @robrap! Thanks again for the review. As @felipemontoya said, we decided to move on with this initial proposal and review event_status in the new PR (here it is by the way) as we thought it could use more discussion.

Again, thank you for the help with the docs!

event_type="org.openedx.learning.student.registration.completed.v1",
data={
"user": UserData,
}
)


# .. event_type: org.openedx.learning.auth.session.login.completed.v1
# .. event_name: SESSION_LOGIN_COMPLETED
# .. event_description: emitted when the user's login process in the LMS is completed.
# .. event_data: UserData
SESSION_LOGIN_COMPLETED = OpenEdxPublicSignal(
event_type="org.openedx.learning.auth.session.login.completed.v1",
data={
"user": UserData,
}
)


# .. event_type: org.openedx.learning.course.enrollment.created.v1
# .. event_name: COURSE_ENROLLMENT_CREATED
# .. event_description: emitted when the user's enrollment process is completed.
# .. event_data: CourseEnrollmentData
COURSE_ENROLLMENT_CREATED = OpenEdxPublicSignal(
event_type="org.openedx.learning.course.enrollment.created.v1",
data={
"enrollment": CourseEnrollmentData,
}
)


# .. event_type: org.openedx.learning.course.enrollment.changed.v1
# .. event_name: COURSE_ENROLLMENT_CHANGED
# .. event_description: emitted when the user's enrollment update process is completed.
# .. event_data: CourseEnrollmentData
COURSE_ENROLLMENT_CHANGED = OpenEdxPublicSignal(
event_type="org.openedx.learning.course.enrollment.changed.v1",
data={
"enrollment": CourseEnrollmentData,
}
)


# .. event_type: org.openedx.learning.course.unenrollment.completed.v1
# .. event_name: COURSE_ENROLLMENT_CHANGED
# .. event_description: emitted when the user's unenrollment process is completed.
# .. event_data: CourseEnrollmentData
COURSE_UNENROLLMENT_COMPLETED = OpenEdxPublicSignal(
event_type="org.openedx.learning.course.unenrollment.completed.v1",
data={
"enrollment": CourseEnrollmentData,
}
)


# .. event_type: org.openedx.learning.certificate.created.v1
# .. event_name: CERTIFICATE_CREATED
# .. event_description: emitted when the user's certificate creation process is completed.
# .. event_data: CertificateData
CERTIFICATE_CREATED = OpenEdxPublicSignal(
event_type="org.openedx.learning.certificate.created.v1",
data={
"certificate": CertificateData,
}
)


# .. event_type: org.openedx.learning.certificate.changed.v1
# .. event_name: CERTIFICATE_CHANGED
# .. event_description: emitted when the user's certificate update process is completed.
# .. event_data: CertificateData
CERTIFICATE_CHANGED = OpenEdxPublicSignal(
event_type="org.openedx.learning.certificate.changed.v1",
data={
"certificate": CertificateData,
}
)


# .. event_type: org.openedx.learning.certificate.revoked.v1
# .. event_name: CERTIFICATE_REVOKED
# .. event_description: emitted when the user's certificate annulation process is completed.
# .. event_data: CertificateData
CERTIFICATE_REVOKED = OpenEdxPublicSignal(
event_type="org.openedx.learning.certificate.revoked.v1",
data={
"certificate": CertificateData,
}
)


# .. event_type: org.openedx.learning.cohort_membership.changed.v1
# .. event_name: COHORT_MEMBERSHIP_CHANGED
# .. event_description: emitted when the user's cohort update is completed.
# .. event_data: CohortData
COHORT_MEMBERSHIP_CHANGED = OpenEdxPublicSignal(
event_type="org.openedx.learning.cohort_membership.changed.v1",
data={
"cohort": CohortData,
}
)