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

feat: add new program certificate events #253

Merged
merged 1 commit into from
Aug 2, 2023
Merged

feat: add new program certificate events #253

merged 1 commit into from
Aug 2, 2023

Conversation

justinhynes
Copy link
Contributor

@justinhynes justinhynes commented Jul 26, 2023 •
edited
Loading

Description:

Internal 2U ticket: APER-2625

This PR adds new OpenEdxPublicSignal events triggered by program certificate generation and revocation.

  • Adds a PROGRAM_CERTIFICATE_AWARDED event
  • Adds a PROGRAM_CERTIFICATE_REVOKED event
  • Adds new ProgramCertificateData and ProgramData data classes to support the new events

ISSUE: #250

Reviewers:

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 at this time.

@justinhynes justinhynes requested a review from a team as a code owner July 26, 2023 13:22
@justinhynes justinhynes force-pushed the APER-2625_add-new-program-cert-events branch 2 times, most recently from 860add2 to 5219e7d Compare July 26, 2023 14:00
@robrap
Copy link
Contributor

robrap commented Jul 31, 2023

@justinhynes

  1. [tip] Add fixup! or squash! to your commit title to get a passing Lint Commit Messages check before squashing. Or, if you like it to be red to remind you to squash, ignore my tip. 😉
  2. I added waiting for eng review label for arch-bom review. I'm trying this process out. Let me know if this ends up being a process problem. Thanks.

@robrap robrap added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jul 31, 2023
@justinhynes
Copy link
Contributor Author

Thanks @robrap. Appreciate the tip. I'm going to wait for some additional feedback so folks can follow any additional updates/commits.

Question: Would a review from arch-bom satisfy the group tagged on the PR (hooks-extension-framework)? Or is this a separate group I also have to find/request to take a look? I'm not really sure who I should grabbing for an "official" review.

@robrap
Copy link
Contributor

robrap commented Aug 1, 2023

Question: Would a review from arch-bom satisfy the group tagged on the PR (hooks-extension-framework)? Or is this a separate group I also have to find/request to take a look? I'm not really sure who I should grabbing for an "official" review.

@mariajgrimaldi @felipemontoya?

@felipemontoya
Copy link
Member

@mariajgrimaldi and I are requested review automatically for being the only members of the maintainers group. We try to be quick to address this reviews, but sometimes we aren't.
I believe however that anyone that is a core contributor to this repo would qualify to give reviews. If they are unsure we can always be tagged specifically.

This particular PR I just reviewed and It looks good and all outstanding comments have been addressed so I'll leave the approval for when the Linting check passes.

felipemontoya
felipemontoya approved these changes Aug 2, 2023
View reviewed changes
Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

The definitions and naming look good to me. The Avro part I can't verify but it looks reasonable.

@justinhynes
Copy link
Contributor Author

Thanks for the info and looking over the changes @felipemontoya!

@justinhynes
feat: add new program certificate events
a61fefb 
GitHub Issue: openedx#250
Internal 2U ticket: APER-2625

This PR adds new `OpenEdxPublicSignal` events triggered by program certificate generation and revocation.

- Adds a `PROGRAM_CERTIFICATE_AWARDED` event
- Adds a `PROGRAM_CERTIFICATE_REVOKED` event
- Adds new `ProgramCertificateData` and `ProgramData` data classes to support the new events
@justinhynes justinhynes force-pushed the APER-2625_add-new-program-cert-events branch from 02ec9dd to a61fefb Compare August 2, 2023 12:43
@justinhynes
Copy link
Contributor Author

I squashed the commits and all checks have passed. This should be ready to merge whenever someone with permission has time. Thanks in advance!

@robrap robrap merged commit 27ed4da into openedx:main Aug 2, 2023
@justinhynes justinhynes deleted the APER-2625_add-new-program-cert-events branch August 2, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robrap robrap robrap left review comments

@rgraber rgraber rgraber left review comments

@hurtstotouchfire hurtstotouchfire hurtstotouchfire left review comments

@jsnwesson jsnwesson jsnwesson approved these changes

@felipemontoya felipemontoya felipemontoya approved these changes

Assignees
No one assigned
Labels
waiting for eng review PR is ready for review. Review and merge it, or suggest changes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@justinhynes @robrap @felipemontoya @hurtstotouchfire @rgraber @jsnwesson