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

#4466 Enrich /webhooks/deliveries endpoint to include webhook endpoint schema #4802

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

smokyabdulrahman
Copy link

@smokyabdulrahman smokyabdulrahman commented Jan 7, 2025

[WIP]
Addresses #4466

P.S.
I believe backend changes should be deployed first before deploying frontend ones, so there would be no downtime/errors window as a result of frontend updating first.

Copy link

vercel bot commented Jan 7, 2025

@smokyabdulrahman is attempting to deploy a commit to the polar-sh Team on Vercel.

A member of the Team first needs to authorize it.

@smokyabdulrahman smokyabdulrahman changed the title #4466 Show endpoint url in event log table #4466 Enrich /webhooks/deliveries endpoint to include webhook endpoint Jan 7, 2025
@smokyabdulrahman smokyabdulrahman changed the title #4466 Enrich /webhooks/deliveries endpoint to include webhook endpoint #4466 Enrich /webhooks/deliveries endpoint to include webhook endpoint schema Jan 7, 2025
Comment on lines +131 to +133
webhook_endpoint: WebhookEndpoint = Field(
description="The webhook endpoint called by this delivery."
)
Copy link
Author

Choose a reason for hiding this comment

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

Is returning the whole schema too much?
Should I be adding a webhook_endpoint_url: EndpointUrl instead and load it like:

[WebhookDeliverySchema.model_validate({**result, "webhook_endpoint_url": result.webhook_endpoint.url) for result in results],

@smokyabdulrahman smokyabdulrahman marked this pull request as ready for review January 7, 2025 13:46
@Mizokuiam
Copy link

Thank you for raising this issue! I'll look into it and try to help if I can.

@smokyabdulrahman
Copy link
Author

Thank you for raising this issue! I'll look into it and try to help if I can.

Thank you so much.

I've raised some questions on the original issue. So maybe after answering those questions, the solution might change.

#4466 (comment)

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