feat: add necessary tooling for Open edX events

[mariajgrimaldi]

Description:
This PR adds the tooling needed to create and trigger events in Open edX platform. This PR includes:

• OpenEdxPublicSignal class: used to define events with the relevant metadata and methods to trigger them on the Open edX platform.
• Custom Exceptions: raised by the tooling when there's an error sending or instantiating events

The overall implementation was discussed by members of the community in this PR: #4

JIRA:
https://edunext.atlassian.net/browse/PS2021-629?atlOrigin=eyJpIjoiODIwNDA5M2MxZGRlNGQ2NjhjNjJhZWIwNWZiNWNlYjUiLCJwIjoiaiJ9

Testing instructions:

Option 1: using PR #18

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 eduNEXT:MJG/1st_batch_openedx_events
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:

• tag reviewer
• tag reviewer

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:

[mariajgrimaldi]

@felipemontoya this is what I'm returning for spec version.
The OEP 41 says: is a mandatory field that refers to the version of CloudEvents. We have to use “1.0” to be spec-compliant.
In our case, we'd refer to openedx-events version, right?

[felipemontoya]

I like the idea of sending the version of the openedx_events library, but I would not like to mix it with the specversion of CloudEvents.
I'd rather leave specversion alone as "1.0" to not mess around and send another field with the openedx events version.

For the new field I'm thinking events_version, but it could be other key

[ormsbee]

You could also play off the source* convention and call it sourcelib or something, but I'm totally fine with events_version as well.

[mariajgrimaldi]

Hey @felipemontoya! I hope you're doing great.

I found an interesting case while implementing the POC with the 8 events. When the platform sends the signal UNENROLL_DONE, sends an argument called skip_refund. I thought it was not a big deal, I could send the argument as a keyword... but the problem is this validation -that was discussed in the naming ADR-:

We expect send calls to always send things over as keyword args, and throw an exception if we get any parameters that we don't expect, or any parameters that are missing, based on the constructor call to OpenEdxPublicSignal.

This prevents sending extra arguments not defined in the event definition. And skip_refund is not part of the CourseEnrollment object so it doesn't make sense to include it in the CourseEnrollmentrData class.

Then what can we do? I was thinking about just checking that at least the arguments defined when instantiating the event are sent, what do you think? This way we can actually send sender when needed.

[felipemontoya]

My understanding of the spirit of the discussion we had in the naming ADR is that we precisely dont want to permit extra params. We want to make the events super explicit in what they are passing around.

In my opinion we have 2 options. We add skip_refund to make the event we are creating for unenroll an make it 100% compatible to the current unenroll_done or we don't and we make an event that is not 100% compatible. This means we will add it to the core platform and it will co-exist with the current unenroll_done for a while until we can migrate things.

I'd be inclined to say that we should leave without skip_refund and send two events when skip_refund is false. One for the unenrollment and one for the refund.

[mariajgrimaldi]

Do you mean something like:

UNENROLL_DONE_WITH_REFUND
UNENROLL_DONE_WITHOUT_REFUND

And the receivers would be subscribed to both signals, right? I like the idea, but what if more extra arguments are needed in the future? How many variations will we need?

[felipemontoya]

I mean something like:

UNENROLL_DONE-> this is the unenrollment event with skip_refund = true
REFUND -> this is what happens when skip_refunds = false

[feanil]

I think I would do the following:

UNENROLL_DONE -> this event gets sent on any unenrollment whether or not there is a refund.
REFUND -> this event gets sent if skip_refunds = false.

In the case of a refund, you still want the unenroll message for anyone that cares about enrollments.

[ormsbee]

A few comments:

Defining Metadata in data.py
Can the metadata itself be another attrs class that has well defined fields? This could live somewhere independently of the learning events.

Separating CloudEvents serialization specifics.
Let's keep the metadata here in a more natural Python form while it's in memory, and convert it to the serialized version in an explicit CloudEvents layer in the future, when that becomes relevant. In concrete terms, I think that just means that specversion goes away at this layer (it's a fixed constant for CloudEvents), time becomes a UTC datetime instead of a string, and events version might become a tuple of ints so that version comparisons don't have to worry about lexical ordering of strings (e.g. '0.9.0' vs. '0.10.0').

Message Queue version
This isn't something I think you need to do yet, but is the long term plan to make metadata an optional param that's auto-generated in the case of a local send, but populated from a serialized message if the signal is transmitted over a message queue?

UUID?
The CloudEvent will eventually have a UUID. Is it useful to have it in this metadata as well?

Nit: "get" vs. "create/generate"
This is a nit, but for me, get implies something is more or less staying the same and you're accessing it, while create or generate means that you're creating a new thing and handing it back.

[mariajgrimaldi]

Hey there @ormsbee! I hope you're doing good!

I've updated the PR with your recommendations:

Defining Metadata in data.py
I've created this PR showing how I would do this

Separating CloudEvents serialization specifics.
I followed all your recommendations here.

Message Queue version
I think the best thing is to consider this use case already. If a Metadata object exists, then it's not generated. What do you think?

UUID?
I added an ID as part of the metadata object, based on what the OEP-41 said:
https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#id2

Nit: "get" vs. "create/generate"
Thanks for the comment! I fixed it.

[mariajgrimaldi]

Update: we fully adopted defining metadata in data.py

[felipemontoya]

Very good work @mariajgrimaldi.

I think this PR already covered all the feedback and the current proposal for the tooling is very robust. On my opinion we are ready to merge this.

[robrap]

@mariajgrimaldi: Hello Maria. It seems we went with a tuple instead of a string. I'm curious why we did that. I'm working on the event bus, which is going to need to send this metadata across the wire, so I may need to pass in a string and convert it back to a tuple, which will be a bit strange, but would make the type in this comment more correct.

[mariajgrimaldi]

@robrap Hello there, Robert. After some digging, I found this discussion I had with dave when this PR was open: #7 (comment) If you think there's room for improvement, let me know! -besides updating the docstring 😅 -

[robrap]

Thank you, Maria. I'm updating in this PR: https://github.com/openedx/openedx-events/pull/168/files#r1083076265