[BD-32] Hooks framework documentation #27157

felipemontoya · 2021-03-26T22:24:02Z

Description

This PR adds a document to the docs/guides directory that explains the generalities of the hooks framework, gives examples of usage and lists the available events so far.

We created a supporting library that is a complete and functional example and we mention it in the document: https://github.com/eduNEXT/openedx-events-2-zapier/

Testing instructions

The main objective of the PR is to add the documentation. No testing is necessary other than the rst docs compiling correctly.

Deadline

We already merged the openedx-events part of the hooks framework, this PR should be merged before maple is cut so that the documentation is present for developers that want to try it out.

openedx-webhooks · 2021-03-26T22:24:12Z

Thanks for the pull request, @felipemontoya! I've created BLENDED-808 to keep track of it in Jira. More details are on the BD-32 project page.

When this pull request is ready, tag your edX technical lead.

docs/guides/hooks/index.rst

arch-bom-gocd-alerts · 2021-06-11T18:02:23Z

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 fmo/initial_hook_framework_docs && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

felipemontoya · 2021-09-29T22:51:10Z

I updated this PR so that it reflects the actual status of the openedx-events usage.

Tagging some people that might be interested in this docs.
@regisb @robrap @bradenmacdonald @ormsbee and @mariajgrimaldi

docs/guides/hooks/index.rst

felipemontoya · 2021-10-01T17:41:47Z

@diegomillan fyi

robrap · 2021-10-01T18:54:08Z

docs/guides/hooks/index.rst

+   * - `STUDENT_REGISTRATION_COMPLETED <https://github.com/eduNEXT/openedx-events/blob/main/openedx_events/learning/signals.py#L18>`_
+     - org.openedx.learning.student.registration.completed.v1
+     - Active | `location <https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/user_authn/views/register.py#L258>`_


This doesn't need to be a blocker to these initial manual docs, but what do you all think of instead (or in addition) adding the following above the send_event call?:

# .. event_implemented_name: STUDENT_REGISTRATION_COMPLETED

We could then auto-generate this section using a sphinx plugin.

We could easily pull in the line info of the implementation.

We could add linting to ensure any imported events get annotated.

It would be trickier (but I assume possible?) to pull the definition information as well. Or, the doc could instead just have a single link to the published document of event definitions.

Note: The current manual docs are a little risky in terms of event definition version information, because that is related to the version of the openedx-events library that happens to be used at the time. Do the docs need reminders on when to update docs, or do you think that is all obvious?

I would be very happy about something that lets us auto-generate this docs with sphinx.

Adding the event_implemented_name annotation would be great to make sure that the docs are up to date. I had thought about writing a script for this doing string search only, but adding the annotation and the linting is way more robust I think.

About 3) I don't know enough about sphinx plugins to know if pulling the definition annotations is easy or even possible, but from the perspective of a developer reading this docs I think that would be the best outcome.

We could also link the definitions to the specific version of the openedx-events library that the platform currently uses. This way there is no slow drift in the exact line where the definition is written.

About the reminders of when to update this docs, I really think that after a period of rapid growth in the available events it will stabilize and will change very seldom. I think as long as we update this once before releases are cut we are sufficiently on the clear. I suppose it depends on how automatic we can make it.

Hi there @robrap

After you mentioned docs generation in PR Events Definition, I went ahead and implemented this:

Sphinx plugin for events annotations: [BD-32] docs: add documentation for Open edX Events code-annotations#72

Code annotations config: feat: add sphinx configuration for Open edX Events eduNEXT/edx-platform#236

Pylint linter for events annotations: feat: add lint checker for Open edX Events code-annotations edx-lint#176

It's a bit rough on the edges, but I think it could work as a POC. What do you think?

How it looks after running make technical-docs:

@mariajgrimaldi: I look forward to checking that out more soon. I'm working on getting Django upgrade over the line, so it might be a bit. Given that, I just want to confirm that your post is just about the already existing annotations for the event definitions, and isn't yet a response to my note about annotating the implementations, right? Thanks.

Thanks for answering @robrap. Yes, they are based on the existent annotations :)

@mariajgrimaldi: In general, looks good. Let me know when they are ready for review and I'll either review or help find a reviewer. Thanks.

felipemontoya · 2021-10-06T17:30:57Z

@robrap seeing how the automatic docs will take more time and how having some documentation that gets the ball rolling on usage would be necessary before maple is cut, would you approve that we merge this now?

robrap

@felipemontoya: My notes around automating are all non-blocking ideas for the future. Although, we could consider adding the annotations sooner rather than later, even if we don't yet automate documentation around them. However, even the annotation idea is non-blocking for this PR.

robrap · 2021-10-06T18:33:02Z

docs/guides/hooks/index.rst

+
+   * - `SESSION_LOGIN_COMPLETED <https://github.com/eduNEXT/openedx-events/blob/main/openedx_events/learning/signals.py#L30>`_
+     - org.openedx.learning.auth.session.login.completed.v1
+     - Active | `location <https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/user_authn/views/login.py#L306>`_


[Before merging] I am guessing these links will break because they all share the same text (location), but have different URLs.

Please test this and the rest of the changes by building the docs locally.

You can probably fix these by making them anonymous using a double-underscore at the end (e.g. __).

[Non-blocking feedback] Also, I wonder if we could come up with a replacement for the word location? Because the term is general, it is unclear which link will bring you to the definition and which to the implementation, until you follow them of course. How do you feel about event trigger, or something like that?

I did not know this about the rst links. Thanks. I tested running make technical-docs and it build in my devstack.

I added a note at the beginning of the list about the two links and changed the 'location' for a date added. This also removes the status since we really don't have a clear expectation of what goes there or what does active mean. Instead I'm writing the date the event was added to the platform with the idea that a developer sees it and they can figure if the event has been in the platform for long it is likely more stable.

This idea about the dates is something we could further extend to the annotations in the openedx-events library. The moment an event is created, we can put an experimentan_until annotation. This way, even if the original dev does not go back to that repo to say the event is no longer experimental, but fully supported or active a dev looking at this in the future can extrapolate from the date.

I wanted to get this up here to be able to comment today. Tomorrow I'll add the annotations to the links since I'm closing my working day now.

robrap · 2021-10-06T19:00:03Z

docs/guides/hooks/index.rst

+
+Index of Events
+-----------------
+


[Non-blocking idea] Consider adding brief introductory text. Things to possibly mention:

The name of each event is linked to its definition.

The location links to the event trigger implementation.

How will status be used? Do we imagine this will eventually be Active or Deprecated?

Maybe note that this index represents the subset of Open edX events implemented in edx-platform?

I added the introduction. About 3. I removed it with the rationale from the other comment.

felipemontoya · 2021-10-08T15:04:32Z

@robrap you are right about the dates. I changed to the standard format used in toggles.

Also I added the annotations at the triggers.

Do I have your approval to merge when the tests passes?

felipemontoya · 2021-10-08T15:30:15Z

@BbrSofiane I'd like to have this documentation merged before we tag the Maple branches. I'm waiting for the green build and an official review approval. Are those branches being tagged today?

BbrSofiane · 2021-10-08T15:37:57Z

@felipemontoya It should be fine, we are tagging the branches next Friday.

regisb · 2021-10-11T08:56:43Z

docs/guides/hooks/index.rst

+use them to change the functionality of the platform as needed and still be able
+to migrate to newer open releases with very little to no development effort. In
+the case of the events, this is detailed in the `versioning ADR`_ and the
+`payload ADR`_.


I found this description of events vs filters very clear and informative. Could you please add some information about the technical implementation? In particular, I think that users would need to know whether events are synchronous or asynchronous.

I'm happy you found it clear and informative. I added a paragraph with technical considerations below this one.

Includes: - general documentation - links to individual events definitions and location - adding examples to events docs - adding annotations at the trigger location

robrap

Thanks!

edx-status-bot · 2021-10-13T21:05:42Z

Your PR has finished running tests. There were no failures.

openedx-webhooks · 2021-10-14T15:39:34Z

@felipemontoya 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

felipemontoya · 2021-10-14T15:39:51Z

Thanks to everyone for the comments and for helping us make this a better started documentation.

edx-pipeline-bot · 2021-10-14T16:38:20Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-10-14T17:49:28Z

EdX Release Notice: This PR has been deployed to the production environment.

edx-pipeline-bot · 2021-10-14T17:49:28Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Mar 26, 2021

felipemontoya mentioned this pull request Mar 26, 2021

OEP-50 | initial draft of the hooks extension framework openedx/open-edx-proposals#184

Merged

mariajgrimaldi reviewed May 4, 2021

View reviewed changes

docs/guides/hooks/index.rst Outdated Show resolved Hide resolved

felipemontoya force-pushed the fmo/initial_hook_framework_docs branch from 4527957 to 5d7df36 Compare August 20, 2021 22:45

felipemontoya marked this pull request as draft August 20, 2021 22:48

felipemontoya mentioned this pull request Aug 20, 2021

feat: add event_status to inline docs add ADR to support it openedx/openedx-events#24

Draft

felipemontoya force-pushed the fmo/initial_hook_framework_docs branch 2 times, most recently from ec829db to c034bb1 Compare September 29, 2021 22:40

felipemontoya marked this pull request as ready for review September 29, 2021 22:42

felipemontoya changed the title ~~[BD-32 ]Hooks framework documentation~~ [BD-32] Hooks framework documentation Sep 29, 2021

bradenmacdonald reviewed Sep 29, 2021

View reviewed changes

docs/guides/hooks/index.rst Outdated Show resolved Hide resolved

robrap reviewed Oct 1, 2021

View reviewed changes

robrap reviewed Oct 6, 2021

View reviewed changes

felipemontoya force-pushed the fmo/initial_hook_framework_docs branch from cd955b2 to 6207f82 Compare October 8, 2021 15:02

regisb reviewed Oct 11, 2021

View reviewed changes

felipemontoya force-pushed the fmo/initial_hook_framework_docs branch from 6207f82 to c947616 Compare October 13, 2021 20:10

docs: adding the first version of documentation for open edx events

678c159

Includes: - general documentation - links to individual events definitions and location - adding examples to events docs - adding annotations at the trigger location

felipemontoya force-pushed the fmo/initial_hook_framework_docs branch from c947616 to 678c159 Compare October 13, 2021 20:28

robrap approved these changes Oct 13, 2021

View reviewed changes

felipemontoya merged commit f12ad2f into openedx:master Oct 14, 2021

openedx-webhooks added merged and removed needs triage labels Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BD-32] Hooks framework documentation #27157

[BD-32] Hooks framework documentation #27157

felipemontoya commented Mar 26, 2021 •

edited

Loading

openedx-webhooks commented Mar 26, 2021 •

edited

Loading

arch-bom-gocd-alerts commented Jun 11, 2021

felipemontoya commented Sep 29, 2021

felipemontoya commented Oct 1, 2021

robrap Oct 1, 2021

felipemontoya Oct 1, 2021

mariajgrimaldi Oct 4, 2021 •

edited

Loading

robrap Oct 4, 2021

mariajgrimaldi Oct 4, 2021

robrap Oct 6, 2021

felipemontoya commented Oct 6, 2021

robrap left a comment

robrap Oct 6, 2021

felipemontoya Oct 6, 2021

felipemontoya Oct 6, 2021

robrap Oct 6, 2021

felipemontoya Oct 6, 2021

felipemontoya commented Oct 8, 2021

felipemontoya commented Oct 8, 2021

BbrSofiane commented Oct 8, 2021

regisb Oct 11, 2021

felipemontoya Oct 13, 2021

robrap left a comment

edx-status-bot commented Oct 13, 2021

openedx-webhooks commented Oct 14, 2021

felipemontoya commented Oct 14, 2021

edx-pipeline-bot commented Oct 14, 2021

edx-pipeline-bot commented Oct 14, 2021

edx-pipeline-bot commented Oct 14, 2021


		Index of Events
		-----------------

[BD-32] Hooks framework documentation #27157

[BD-32] Hooks framework documentation #27157

Conversation

felipemontoya commented Mar 26, 2021 • edited Loading

Description

Testing instructions

Deadline

openedx-webhooks commented Mar 26, 2021 • edited Loading

arch-bom-gocd-alerts commented Jun 11, 2021

felipemontoya commented Sep 29, 2021

felipemontoya commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipemontoya commented Oct 6, 2021

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipemontoya commented Oct 8, 2021

felipemontoya commented Oct 8, 2021

BbrSofiane commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

edx-status-bot commented Oct 13, 2021

openedx-webhooks commented Oct 14, 2021

felipemontoya commented Oct 14, 2021

edx-pipeline-bot commented Oct 14, 2021

edx-pipeline-bot commented Oct 14, 2021

edx-pipeline-bot commented Oct 14, 2021

felipemontoya commented Mar 26, 2021 •

edited

Loading

openedx-webhooks commented Mar 26, 2021 •

edited

Loading

mariajgrimaldi Oct 4, 2021 •

edited

Loading