-
Notifications
You must be signed in to change notification settings - Fork 364
Create webhook when config is created #1873
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Skipped Deployments
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1873 +/- ##
==========================================
+ Coverage 26.04% 26.29% +0.24%
==========================================
Files 784 788 +4
Lines 54526 54769 +243
Branches 1908 1936 +28
==========================================
+ Hits 14202 14401 +199
- Misses 39955 39999 +44
Partials 369 369 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Differences Found✅ No packages or licenses were added. SummaryExpand
|
@@ -0,0 +1,16 @@ | |||
import type { Stripe } from "stripe"; | |||
|
|||
export const supportedStripeEvents: Array<Stripe.WebhookEndpointCreateParams.EnabledEvent> = [ |
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.
Do we need to support all those events? I'm wondering e.g what should app report payment_intent.created
- maybe we can start with supporting:
payment_intent.succeeded
payment_intent.amount_capturable_updated
Or we want to create webhook with the most events covered and then add case by case? If yes - maybe we can still add support for new event both here and in webhook handler (at least in development).
We should also think if it is possible to migrate existing webhooks e.g if we decided to add new event in the future
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.
I copied them blindly, I will add only the ones we support NOW and we can keep adding
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.
Pull Request Overview
This PR implements webhook creation when a new Stripe configuration is created. Key changes include:
- Adding the new "appUrl" field in the TRPC context and updating webhook URL building.
- Introducing "webhookId" as a required field in Stripe configuration and propagating it through repo, tests, and model validation.
- Enhancing Stripe webhook management and TRPC handlers to support proper webhook creation and Stripe authentication checks.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
apps/stripe/src/modules/trpc/context-app-router.ts | Added "appUrl" from the request header to the context. |
apps/stripe/src/modules/transactions-recording/transaction-recorder-file.ts | Added a check to ensure a transaction exists before proceeding. |
apps/stripe/src/modules/stripe/supported-stripe-events.ts | Defined supported Stripe event types. |
apps/stripe/src/modules/stripe/stripe-webhook-url-builder.ts | Implemented URL builder for Stripe webhooks with proper error handling. |
apps/stripe/src/modules/stripe/stripe-webhook-manager.ts | Created a new manager for handling webhook creation including error categorization. |
apps/stripe/src/modules/stripe/stripe-client.ts | Changed the constructor from private to public for instance creation. |
apps/stripe/src/modules/stripe/stripe-auth-validator.ts | Added auth validation integration with Stripe. |
apps/stripe/src/modules/app-config/trpc-handlers/new-stripe-config-trpc-handler.ts | Integrated webhook creation, URL validation, and authentication checks into the TRPC handler. |
apps/stripe/src/modules/app-config/stripe-config.ts | Updated the StripeConfig model with a new "webhookId" field. |
Other test and repository files | Updated mocks and test cases to cover the new webhookId and webhook URL functionalities. |
Comments suppressed due to low confidence (1)
apps/stripe/src/modules/app-config/stripe-config.test.ts:25
- [nitpick] Ensure that using mockStripeWebhookSecret as the value for webhookId is intentional and aligns with the expected identifier format for a webhook.
webhookId: mockStripeWebhookSecret,
constructor(nativeClient: Stripe) { | ||
this.nativeClient = nativeClient; |
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.
Consider confirming that making the constructor public is intentional, as the previous private constructor enforced controlled instance creation via createFromRestrictedKey.
constructor(nativeClient: Stripe) { | |
this.nativeClient = nativeClient; | |
private constructor(nativeClient: Stripe) { |
Copilot uses AI. Check for mistakes.
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.
made it public for easier testing, this class is just a wrapper
Scope of the PR
Related issues
Checklist