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

Jump to bottom

feat: add event_status to inline docs add ADR to support it #24

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: add event_status to inline docs add ADR to support it #24

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
60 changes: 60 additions & 0 deletions docs/decisions/0004-events-status.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
1. Events status
================

Status
------

Draft

Context
-------

Each Open edX Event will evolve according to the needs of the community.
For that reason, with this ADR, we will define a lifecycle that each
event will follow individually.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder that docs shouldn’t use hard line breaks.


Decision
--------

Each Open edX Event will follow the following lifecycle:

State 1. Provisional
Copy link
Contributor

Choose a reason for hiding this comment

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

Some initial thoughts:

  • Would provisional status affect versioning? Would you be more willing to fix issues without bumping versions, or will you have strict versioning while in a provisional state?
  • By what process and who decides when it becomes active?
  • Could an event go from provisional to removed, if it never becomes active?
  • Who would use a provisional event, and what do they need to know about making this choice?
  • What will prevent us from having provisional events forever? Does it require a review deadline? Seems similar to v0 APIs, and I wonder if we still have issues around that in the platform.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @robrap,

Thanks a lot for the thoughtful questions.

Would provisional status affect versioning? Would you be more willing to fix issues without bumping versions, or will you have strict versioning while in a provisional state?

The way we have discussed it, we always need to have strict versioning in place so that plugin developers and core-platform developers only have to agree on the version of this library that is being used. Then both can go their separate ways with a common definition and write testeable code without the plugin requiring the whole edx-platform repo.

Now this does create a big disconnect, because as you pointed in the next question:

By what process and who decides when it becomes active?

The process by which an event becomes active is the actual usage in the platform. By having this event_status annotation we only wanted to signal it in a way that is easy to pick up by an automated process of documentation.

The scenario we are trying to avoid is a developer finding out the definition of an event in this library and using it, only to later never see the event happening because it was not yet implemented in the platform or it was deprecated or replaced or any of the above.

Could an event go from provisional to removed, if it never becomes active?

The wording seems a bit off for me even as non native speaker, but yes. We could change "provisional" to "draft" or "proposal" or something to that effect.

Who would use a provisional event, and what do they need to know about making this choice?

Provisional is a way to mark that the event is fully defined here, but might not be used in the edx-platform yet. A developer that wants to use a provisional event should go to the platform and see if it is in use. If so they should also make a PR here to change it to active.

What will prevent us from having provisional events forever? Does it require a review deadline? Seems similar to v0 APIs, and I wonder if we still have issues around that in the platform.

I think you are correctly pointing out here the eternal problem of keeping the annotations updated. We had a long discussion with @mariajgrimaldi about this. Maybe is even worse that we though because of the versioning process.

I'll ilustrate from the point of view of a developer creating a new event.

At openedx-events:
- Dev creates an event and leave it with provisional status.
- It gets merged
- A semver tag is made e.g. 1.1.0

At edx-platform:
- Dev updates the requirement of openedx-events to 1.1.0
- Dev uses the new event and the change lands in master. In theory this makes the event active, but my 1.1.0 version has it as provisional.

Now Dev needs to make this chores just to keep this updated:
- go to openedx-event and mark it active
- get it merged
- create new semver tag 1.1.1
- go to edx-platform and update to 1.1.1
- get it merged

This is a lot of work and in the meantime anyone with a version of edx-platform that already has the event in full action still gets the message that is provisional in the corresponding library and might not want to use it. The same dev could skip the last chores (not that they should, but it could happen) and leave a worse problem than the lack of information of the event_status would have caused.

I'm thinking now that a better way would be to keep this information closer to the usage of the events in the edx-platform repo instead of the definition in the openedx-events repo. I updated a previous PR with a version of the general hooks framework documentation that lives in edx-platform to reflect how this would look: https://github.com/edx/edx-platform/blob/5d7df3606822cd4e577ea6abeb386194e81e1201/docs/guides/hooks/index.rst.

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 see. I think this helps clarify a lot. Thank you.

  1. Do you think the terms defined and implemented more precisely describe what you had meant by provisional and active? That is what I am hearing in your above comments.
  2. I agree that trying to document the events as implemented next to the definition sounds painful to keep up-to-date and accurate, especially with the versioning issue you described.
  3. It would be nice if documentation for implemented events could also be automated. This might come in the form of:
    a. Automatically finding implementations in code.
    b. Annotating implementations, and finding those annotations.
    c. A runtime API endpoint that pseudo-documents based on what was loaded. Our toggles API endpoint is like this.
    d. Other ideas? The alternative being a manual document that we try to keep up-to-date.
  4. Ultimately, how we handle implemented documentation may lead us in different directions for where we might want to mark events as deprecated. Not sure.

Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Aug 23, 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, thanks for the feedback.

About # 2, we've created a sphinx extension in this PR that pulls the inline code annotations. In previous comments, you mentioned -here- docs generation from dependencies were not possible for now. From what I understand, I understand it's for the path used to generate the docs, am I right? Well, for now, I added a fixed path to generate Open edX Events documentation. Here are the modifications in the Sphinx configuration -for testing purposes-.

If we could use this, what would be the process of updating the documentation? Your insight would be beneficial.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mariajgrimaldi: There are a number of ways to go, and this will take some thinking through. I think you'll need to discuss as part of the community or with Ned and his team.

One idea would be to generate the event definition documentation as part of the openedx-events docs in this repo. This repo could publish its docs to Readthedocs, and edx-platform docs could provide a link (or include?) of this doc.

Additionally, Régis wrote the original Sphinx code for toggles and may have some thoughts and opinions.

Please take this comment as non-blocking food-for-thought that you can discuss with others that may be able to spend more time on this. Good luck, and I hope I was able to help get the ball rolling for you. Thank you.

~~~~~~~~~~~~~~~~~~~~

Events just created and accepted in the repository `openedx-events`,
these events haven't been accepted in Open edX platform or by the community.

State 2. Active
~~~~~~~~~~~~~~~

Events being used by Open edX platform and by the community.

State 3. Deprecated
~~~~~~~~~~~~~~~~~~~

Events that members of the community decide to deprecate in Open edX platform.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention and link to the DEPR process? I imagine this won’t need a unique process.


State 4. Removed
~~~~~~~~~~~~~~~~~

Events that members of the community removed from Open edX platform after
documentation and discussion of the removal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we get to removal, wouldn’t it just be deleted along with its documentation? Would documentation for this (as well as other state changes) be captured in the changelog?


State 5. Replaced
~~~~~~~~~~~~~~~~~

Events that members of the community replaced for another event after
documentation and discussion of the change.


Consequences
------------

1. Each event must carry its state in its code-annotation documentation.

2. Each event must go through each state in order. First, must be created
in this repository with the state `provisional`, when Open edX accepts it
must change to `active` and when the community decides to deprecate it, it
Copy link
Contributor

Choose a reason for hiding this comment

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

Or removed if it isn’t accepted?

must be updated to `deprecated`, then `removed`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Removed or replaced?


1. Each state must be up-to-date.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Each state must be up-to-date.
3. Each state must be up-to-date.
  1. What does this mean exactly? Is it about moving the process forward, or keeping documentation updated, or multiple things?

9 changes: 9 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 @@ -15,6 +15,7 @@
# .. event_name: STUDENT_REGISTRATION_COMPLETED
# .. event_description: emitted when the user registration process in the LMS is completed.
# .. event_data: UserData
# .. event_status: provisional
STUDENT_REGISTRATION_COMPLETED = OpenEdxPublicSignal(
event_type="org.openedx.learning.student.registration.completed.v1",
data={
Expand All @@ -27,6 +28,7 @@
# .. event_name: SESSION_LOGIN_COMPLETED
# .. event_description: emitted when the user's login process in the LMS is completed.
# .. event_data: UserData
# .. event_status: provisional
SESSION_LOGIN_COMPLETED = OpenEdxPublicSignal(
event_type="org.openedx.learning.auth.session.login.completed.v1",
data={
Expand All @@ -39,6 +41,7 @@
# .. event_name: COURSE_ENROLLMENT_CREATED
# .. event_description: emitted when the user's enrollment process is completed.
# .. event_data: CourseEnrollmentData
# .. event_status: provisional
COURSE_ENROLLMENT_CREATED = OpenEdxPublicSignal(
event_type="org.openedx.learning.course.enrollment.created.v1",
data={
Expand All @@ -51,6 +54,7 @@
# .. event_name: COURSE_ENROLLMENT_CHANGED
# .. event_description: emitted when the user's enrollment update process is completed.
# .. event_data: CourseEnrollmentData
# .. event_status: provisional
COURSE_ENROLLMENT_CHANGED = OpenEdxPublicSignal(
event_type="org.openedx.learning.course.enrollment.changed.v1",
data={
Expand All @@ -63,6 +67,7 @@
# .. event_name: COURSE_ENROLLMENT_CHANGED
# .. event_description: emitted when the user's unenrollment process is completed.
# .. event_data: CourseEnrollmentData
# .. event_status: provisional
COURSE_UNENROLLMENT_COMPLETED = OpenEdxPublicSignal(
event_type="org.openedx.learning.course.unenrollment.completed.v1",
data={
Expand All @@ -75,6 +80,7 @@
# .. event_name: CERTIFICATE_CREATED
# .. event_description: emitted when the user's certificate creation process is completed.
# .. event_data: CertificateData
# .. event_status: provisional
CERTIFICATE_CREATED = OpenEdxPublicSignal(
event_type="org.openedx.learning.certificate.created.v1",
data={
Expand All @@ -87,6 +93,7 @@
# .. event_name: CERTIFICATE_CHANGED
# .. event_description: emitted when the user's certificate update process is completed.
# .. event_data: CertificateData
# .. event_status: provisional
CERTIFICATE_CHANGED = OpenEdxPublicSignal(
event_type="org.openedx.learning.certificate.changed.v1",
data={
Expand All @@ -99,6 +106,7 @@
# .. event_name: CERTIFICATE_REVOKED
# .. event_description: emitted when the user's certificate annulation process is completed.
# .. event_data: CertificateData
# .. event_status: provisional
CERTIFICATE_REVOKED = OpenEdxPublicSignal(
event_type="org.openedx.learning.certificate.revoked.v1",
data={
Expand All @@ -111,6 +119,7 @@
# .. event_name: COHORT_MEMBERSHIP_CHANGED
# .. event_description: emitted when the user's cohort update is completed.
# .. event_data: CohortData
# .. event_status: provisional
COHORT_MEMBERSHIP_CHANGED = OpenEdxPublicSignal(
event_type="org.openedx.learning.cohort_membership.changed.v1",
data={
Expand Down