-
Notifications
You must be signed in to change notification settings - Fork 83
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
api.schedule: add C3VOCPublishingWebhook #1770
Conversation
Tested:
|
57c4183
to
106545d
Compare
106545d
to
bff1726
Compare
…t) to overwrite values
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as 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 👍
My two remaining nitpicks would be:
- We don't have much
info
-level logging in the API modules of the codebase so far, so I don't know whether we should include those. - The
try...catch
forKeyError
seems overbroad to me; but I'm unaware of any particular problem that would cause.
This comment was marked as resolved.
This comment was marked as resolved.
98c1eae
to
ac82add
Compare
Another one-more-thing: we should probably add some test coverage for this. I'm prety convinced from manually testing it that it works well, but for the longer-term, some automated coverage would be nice. Borrowing some of the existing test code for the |
46e80fd
to
1a5c786
Compare
1a5c786
to
97ebe3a
Compare
I think my tests cover all the cases we'd want to test. Let me know if i missed any. |
This will allow c3voc to send us video information automatically upon release of videos, as documented here: https://github.com/voc/voctopublish/blob/main/voctopublish/api_client/webhook_client.py#L28-L56