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.

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

Merged
merged 2 commits into from
Jul 19, 2021

Conversation

mariajgrimaldi
Copy link
Member

@mariajgrimaldi mariajgrimaldi commented Jun 8, 2021

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

  1. Go to lms shell make lms-shell
  2. Install openedx-events specifying the events definition branch:
    pip install git+https://github.com/eduNEXT/openedx-events.git@MJG/events_definition#egg=events_definition
  3. Change the Open edX platform branch to MJG/post_register_event
  4. Connect your custom receiver to the signal as follows:
from openedx_events.learning.signals import STUDENT_REGISTRATION_COMPLETED

@receiver(STUDENT_REGISTRATION_COMPLETED)
def callback(**kwargs):
....

We'll add soon an example using our plugin openedx-basic-hooks!

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@mariajgrimaldi mariajgrimaldi changed the title Mjg/events definition [WIP] Mjg/events definition Jun 8, 2021
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/event_data branch 2 times, most recently from 44e043b to 91d3576 Compare June 9, 2021 02:56
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/events_definition branch 2 times, most recently from 7ff3198 to fd51d17 Compare June 9, 2021 03:04
@mariajgrimaldi mariajgrimaldi changed the title [WIP] Mjg/events definition feat: preliminary signal definition Jun 9, 2021
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/events_definition branch 3 times, most recently from 45d7bfa to 81e572f Compare June 9, 2021 17:16
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/event_data branch 2 times, most recently from aa94458 to 5eb16c4 Compare June 9, 2021 17:48
@mariajgrimaldi mariajgrimaldi marked this pull request as ready for review June 10, 2021 18:31
@mariajgrimaldi
Copy link
Member Author

mariajgrimaldi commented Jul 9, 2021

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:

  1. Events tooling PR feat: add necessary tooling for Open edX events  #7: necessary tooling to manage events. Here we implement the class used to send events.
  2. Data attributes for events PR feat: add data attributes definition for learning subdomain #8: definition of the events arguments.
  3. Events definition PR feat: preliminary signal definition #18: definitions of some of the events proposed.

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!

@ormsbee
Copy link

ormsbee commented Jul 9, 2021

Thanks @mariajgrimaldi! @feanil: ^

Comment on lines 53 to 58
COURSE_ENROLLMENT_DEACTIVATED = OpenEdxPublicSignal(
event_type="org.openedx.learning.course.enrollment.deactivated.v1",
data={
"enrollment": CourseEnrollmentData,
}
)
Copy link

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.

Copy link
Member Author

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?

Comment on lines +69 to +74
CERTIFICATE_CHANGED = OpenEdxPublicSignal(
event_type="org.openedx.learning.certificate.changed.v1",
data={
"certificate": CertificateData,
}
)
Copy link

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?

Copy link

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(
Copy link
Contributor

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.

Copy link
Member Author

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!

STUDENT_REGISTRATION_COMPLETED = OpenEdxPublicSignal(
event_type="org.openedx.learning.student.registration.completed.v1",
data={
"user": StudentData,
Copy link

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?

@mariajgrimaldi mariajgrimaldi merged commit 7f468a2 into MJG/event_data Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants