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

docs: add open edx event guide #80

Merged
merged 23 commits into from
Apr 5, 2023
Merged

docs: add open edx event guide #80

merged 23 commits into from
Apr 5, 2023

Conversation

mariajgrimaldi
Copy link
Member

@mariajgrimaldi mariajgrimaldi commented Jul 20, 2022 •
edited
Loading

Description:
This PR adds a how-to for new event creation. Its main purpose is to serve as a guide for new contributions to the library.

Dependencies:
None

Installation instructions:
Just documentation.

Testing instructions:
Just documentation.

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:
What else should we add?

@mariajgrimaldi mariajgrimaldi mentioned this pull request Jul 20, 2022
Closed
robrap
robrap reviewed Jul 20, 2022
View reviewed changes
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thank you! A nice start to kick this off.

docs/how-to/create-new-event.rst Outdated Show resolved Hide resolved
docs/how-to/create-new-event.rst Outdated Show resolved Hide resolved
docs/how-to/create-new-event.rst Outdated Show resolved Hide resolved
@robrap robrap mentioned this pull request Jul 20, 2022
Closed
2 tasks
@mariajgrimaldi
Copy link
Member Author

Hi there @robrap! Thanks for the feedback. I'll be addressing your comments on Monday. Sorry for the delay!

@mariajgrimaldi mariajgrimaldi marked this pull request as ready for review August 12, 2022 19:12
@mariajgrimaldi
Copy link
Member Author

FYI @felipemontoya

@mariajgrimaldi mariajgrimaldi mentioned this pull request Aug 16, 2022
Open
5 tasks
@mariajgrimaldi mariajgrimaldi linked an issue Sep 1, 2022 that may be closed by this pull request
[OEP-55] Improve library documentation #96
Closed
@mariajgrimaldi mariajgrimaldi self-assigned this Oct 7, 2022
@mariajgrimaldi mariajgrimaldi marked this pull request as draft November 17, 2022 14:57
@openedx-webhooks openedx-webhooks added core committer open-source-contribution PR author is not from Axim or 2U labels Nov 17, 2022
@openedx-webhooks
Copy link

openedx-webhooks commented Nov 17, 2022 •
edited
Loading

Thanks for the pull request, @mariajgrimaldi!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

@robrap
Copy link
Contributor

robrap commented Jan 11, 2023

@mariajgrimaldi: Someone recently thoughts some new events defined in this library would be on the event bus. I haven't quite gotten to the bottom of the confusion, but when I went to send a link to some docs, I realized that this doc never landed. I wonder how quickly we could land this, even if we'd want to improve it in the future. Thanks.

@mariajgrimaldi
Copy link
Member Author

@robrap: hello! Thanks for the reminder, sorry I didn't push this forward. A lot of stuff got in the way. Our maintainer team including myself will allocate some time between this week and next to complete this work, at least until its first version.
I'll be tagging you for feedback if you're okay with it! Thanks

@robrap
Copy link
Contributor

robrap commented Jan 11, 2023

Hello @mariajgrimaldi. No apologies necessary. I look forward to helping land something.

@robrap
Copy link
Contributor

robrap commented Feb 1, 2023

@mariajgrimaldi: Another bit we could add is a reminder to document the event in the service where it is produced, using edx-platform as an example: https://github.com/openedx/edx-platform/blob/f01ef48190c7c4ee6d9e06af4abe21e25fe16dff/docs/guides/hooks/events.rst#index-of-events.

Related:

  1. I never remember where to find the index of events. Maybe just having a note in this doc will be a good enough reminder for me? We'll see.
  2. Where is the best place to document that a given event is not only implemented in a service, but is also going to the event bus? Would that just be in an index like the link above? Maybe this depends on if we move this to be part of the event definition? At this point, it is just based on someone adding a listener that sends to the event bus.

This was referenced Feb 1, 2023
Merged
Closed
@mariajgrimaldi mariajgrimaldi changed the base branch from main to MJG/docs-update February 15, 2023 18:58
@mariajgrimaldi mariajgrimaldi changed the title [WIP] docs: add draft of create a new event guide docs: add draft of create a new event guide Feb 15, 2023
@mariajgrimaldi mariajgrimaldi marked this pull request as ready for review February 15, 2023 20:15
@mariajgrimaldi
Copy link
Member Author

I'm finally setting aside some time to do this! Thank you so much for your patience.

@mariajgrimaldi
Copy link
Member Author

@robrap:

Another bit we could add is a reminder to document the event in the service where it is produced, using edx-platform as an example: https://github.com/openedx/edx-platform/blob/f01ef48190c7c4ee6d9e06af4abe21e25fe16dff/docs/guides/hooks/events.rst#index-of-events.

Consider it done!

Where is the best place to document that a given event is not only implemented in a service but is also going to the event bus? Would that just be in an index like the link above? Maybe this depends on if we move this to be part of the event definition? At this point, it is just based on someone adding a listener that sends to the event bus.

I added a how-to called Adding a new event to the event bus, what I think we could do is add an extra step to How to add an Open edX Event to a service saying Add event to the event to the event bus if supported and linking it to the how-to.

Also, we could modify that index with a column saying "sent over the event bus" that says yes or no. What do you think?

@mariajgrimaldi mariajgrimaldi changed the title docs: add draft of create a new event guide docs: add open edx event guide Feb 15, 2023
@mariajgrimaldi
Copy link
Member Author

@robrap: I started addressing your comments! Thank you! I'll let you know when this is ready for review again

@mariajgrimaldi
Copy link
Member Author

@robrap: thank you so much! I think I addressed all your comments. Let me know what you think

@mariajgrimaldi
Copy link
Member Author

I'll be fixing the failing tests later 😅

@robrap
Copy link
Contributor

robrap commented Mar 29, 2023

Thank you for all the work. I’m not sure if I’ll get to review before the end of the conference or not.

robrap
robrap reviewed Mar 31, 2023
View reviewed changes
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks again @mariajgrimaldi.

docs/how-tos/creating-new-events.rst Outdated Show resolved Hide resolved
docs/how-tos/using-events.rst Outdated

In case you are listening to an event in the edx-platform repo, you can directly
use the django syntax since the apps.py method will not be available without the
plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Thanks. @ormsbee can let us know if he'd like this to be even stronger, but either way, that doesn't need to block this PR.

docs/quickstarts/use-events-to-call-webhook.rst Show resolved Hide resolved
docs/concepts/hooks-extension-framework.rst Outdated Show resolved Hide resolved
docs/decisions/0003-events-payload.rst Outdated Show resolved Hide resolved
docs/how-tos/adding-events-to-a-service.rst Outdated Show resolved Hide resolved
docs/how-tos/creating-new-events.rst Show resolved Hide resolved
@mariajgrimaldi mariajgrimaldi mentioned this pull request Apr 4, 2023
Closed
@mariajgrimaldi
Copy link
Member Author

@robrap: again, thank you so much for the help! and the patience 😅

robrap
robrap reviewed Apr 5, 2023
View reviewed changes
docs/quickstarts/index.rst Outdated Show resolved Hide resolved
robrap
robrap approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Looks great. We can always iterate as-needed. Thanks so much for all the great work!

Not sure if you need to kick off tests again to see if openedx/cla check will work?

@mariajgrimaldi mariajgrimaldi merged commit 20ecb70 into main Apr 5, 2023
@mariajgrimaldi mariajgrimaldi deleted the MJG/create-new-event-docs branch April 5, 2023 18:08
@openedx-webhooks
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgraber rgraber rgraber left review comments

@robrap robrap robrap approved these changes

Assignees

@mariajgrimaldi mariajgrimaldi

Labels
core committer open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[OEP-55] Improve library documentation
4 participants
@mariajgrimaldi @openedx-webhooks @robrap @rgraber