-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: Add enterprise signal for learner credit fulfillment revokation #380
Conversation
8629366
to
b9d781c
Compare
...tests/schemas/org+openedx+enterprise+learner_credit_course_enrollment+revoked+v1_schema.avsc
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with a couple docs-related nits and a sanity check type question.
b9d781c
to
c13054e
Compare
@mariajgrimaldi please take a look when you have a chance! |
c13054e
to
f00926a
Compare
@pwnage101 it'll probably be useful for Maria or others in the community to paste in some context from ENT-9213 (which they won't be able to access). Also maybe add links to the edx-enterprise models that this new signal-data correspond to. |
Fair, so first here's the ticket body for ENT-9213:
|
Here's a list of all other openedx PRs which are all part of this same ticket: |
Hi folks! Thank you for your contribution. I'll be reviewing this as soon as I can. Thanks for the patience! |
FYI, I also updated the docstrings with direct links to django models in openedx code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few non-blocking comments. Thanks for the patience!
f52a758
to
9611bf9
Compare
@mariajgrimaldi please merge at your earliest convenience, as I do not have write access to this repository. |
ENT-9213