feat: preliminary signal definition

[mariajgrimaldi]

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 (merged)

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 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:

• @felipemontoya
• @andrey-canon

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:
None for now

[mariajgrimaldi]

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?

[robrap]

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?

[mariajgrimaldi]

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?

[robrap]

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.

[mariajgrimaldi]

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?

[felipemontoya]

@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

[robrap]

@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.

[robrap]

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

[felipemontoya]

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.

[mariajgrimaldi]

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!

[robrap]

Thanks for getting the docs started!

[robrap]

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?

[robrap]

Missing spaces between .. and annotation names.

[robrap]

Since we allow for 120 chars, my preference would be to make this more readable by not wrapping unless we get into long descriptions.

[mariajgrimaldi]

I'll update the repo tox configurations, so we also support 120 chars

[mariajgrimaldi]

Hey! I was about to change max-doc-length = 79 for max-doc-length = 120 but then I found this: https://edx.readthedocs.io/projects/edx-developer-guide/en/latest/style_guides/python-guidelines.html#syntax-and-organization
Do you think it is worth it?

[robrap]

I'll leave it to you if you don't want to make that change, but note that they mention keeping it to 79 to make it more readable, and in this case, I think it makes it less readable.

If you end up keeping the wrapping, please add additional spaces to the wrapped line. For example:

# .. event_description: emitted when the user registration process in the LMS
#    is completed.

Thank you.

[mariajgrimaldi]

Hello @robrap, we moved forward with your recommendation! Thank you so much. Also, we started working on the steps described here: https://code-annotations.readthedocs.io/en/latest/contrib/how_to/add_new_annotations_and_extracted_docs.html

[robrap]

Great. Thank you. I hope you find it useful. One warning, that maybe we should add to the doc, is that we don't yet have a solution for pulling these annotations from dependencies. I'm not sure when and if this would apply to these annotations in particular, but something to keep in mind. It is a future enhancement that could be added later.

[mariajgrimaldi]

Thanks for the heads up 👍 we'll look into it!

[mariajgrimaldi]

This PR was opened because #9 was a PR to an intermediate branch. Please refer to that PR for more discussion.

[felipemontoya]

On the definion of the signals front, I think we are done and ready to merge.

Great work @mariajgrimaldi.

The event_status field definition will be dealt with in a different PR so as to not block the progress of the first examples of usage.