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

docs: [FC-0074] make adjustments to generate docs with sphinx for GH pages #11

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

mariajgrimaldi
Copy link
Collaborator

Description

This PR changes the structure of the doc so it can be rendered in GH pages: https://edunext.github.io/openedx-events-2-zapier/ for the developer's consumption.

@mariajgrimaldi mariajgrimaldi changed the title docs: make adjustments to generate docs with sphinx for GH pages docs: [FC-0074] make adjustments to generate docs with sphinx for GH pages Jan 30, 2025
Copy link

@BryanttV BryanttV left a comment

Choose a reason for hiding this comment

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

Hi @mariajgrimaldi, I was reviewing the documentation and I have a few comments:

  1. The event name link and event type are incorrect:
    | `PERSISTENT_GRADE_SUMMARY_CHANGED`_ | org.openedx.learning.course.persistent_grade.summary.v1 | Triggered when a persistent grade summary is updated. This happens |
  2. When I try to access the handlers.py link it redirects me to the same page, but not to the repository. I think all relative links are broken.
    In the file `handlers.py`_, handlers listen to Django signals using the standard `receiver`_ decorator:

@mariajgrimaldi
Copy link
Collaborator Author

mariajgrimaldi commented Jan 30, 2025

Thanks, @BryanttV. I see that handlers.py works fine in the readme file but not with Sphinx. I'll try a workaround to fix it.

README.rst Outdated
@@ -56,7 +58,7 @@ Supported Events
+-------------------------------------+------------------------------------------------------------+---------------------------------------------------------------------+
| `COURSE_ENROLLMENT_CREATED`_ | org.openedx.learning.course.enrollment.created.v1 | Triggered upon successful course enrollment. |
+-------------------------------------+------------------------------------------------------------+---------------------------------------------------------------------+
| `PERSISTENT_GRADE_SUMMARY_CHANGED`_ | org.openedx.learning.course.persistent_grade.summary.v1 | Triggered when a persistent grade summary is updated. This happens |
| `PERSISTENT_GRADE_SUMMARY_CHANGED`_ | org.openedx.learning.course.persistent_grade_summary.v1 | Triggered when a persistent grade summary is updated. This happens |

Choose a reason for hiding this comment

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

Suggested change
| `PERSISTENT_GRADE_SUMMARY_CHANGED`_ | org.openedx.learning.course.persistent_grade_summary.v1 | Triggered when a persistent grade summary is updated. This happens |
| `PERSISTENT_GRADE_SUMMARY_CHANGED`_ | org.openedx.learning.course.persistent_grade_summary.changed.v1 | Triggered when a persistent grade summary is updated. This happens |

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sorry for all the back and forth, I didn't notice these changes were wrong. Thanks :)

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Could we update the LICENSE and CODEOWNERS link? It is the same problem with the link.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, done!

@mariajgrimaldi
Copy link
Collaborator Author

FYI @sarina

Copy link

@BryanttV BryanttV left a comment

Choose a reason for hiding this comment

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

LGTM!

@mariajgrimaldi mariajgrimaldi merged commit 3a4739c into main Feb 6, 2025
4 checks passed
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.

2 participants