-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improve Stripe payment provider logic #505
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
base: main
Are you sure you want to change the base?
Conversation
| effect: { kind: 'credits', amount: 10 }, | ||
| }, | ||
| }; | ||
| } as const satisfies Record<PaymentPlanId, PaymentPlan>; |
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.
Allows us to do e.g. paymentPlans.credits10.effect.amount which would throw before.
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.
Nice change!
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 simplifies and hardens the Stripe integration by moving from custom Zod parsing to Stripe’s typed events, shifting one-time payments to be invoice-driven, and generating customer-portal sessions per user instead of relying on a static URL.
- Switch webhook processing to use Stripe typed events; remove custom webhookPayload parsing.
- Enable invoices for one-time payments and handle credits via invoice.paid.
- Introduce stripeClient with explicit API version; generate customer-portal session URLs server-side.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| template/app/src/shared/utils.ts | Minor cleanup: unused param naming for assertUnreachable. |
| template/app/src/payment/stripe/webhookPayload.ts | Removed custom Zod-based webhook parsing in favor of Stripe types. |
| template/app/src/payment/stripe/webhook.ts | Rewrote webhook handling to use Stripe typed events; added invoice-driven flows and helper extractors. |
| template/app/src/payment/stripe/stripeClient.ts | Introduced stripeClient with centralized API version constant and docs. |
| template/app/src/payment/stripe/paymentProcessor.ts | Generate billing portal sessions per user; ensure Stripe customer creation; minor API refinements. |
| template/app/src/payment/stripe/paymentDetails.ts | Split update into one-time vs subscription paths; updated shapes and fields. |
| template/app/src/payment/stripe/checkoutUtils.ts | Ensure/create Stripe customer; enable invoice creation for one-time checkouts; align with stripeClient. |
| template/app/src/payment/plans.ts | Strengthened typing for paymentPlans using satisfies and as const. |
| template/app/src/analytics/stats.ts | Switched to stripeClient usage for listing balance transactions. |
| template/app/.env.server.example | Removed obsolete STRIPE_CUSTOMER_PORTAL_URL. |
| opensaas-sh/blog/src/content/docs/guides/payments-integration.mdx | Updated docs to reflect customer-portal and customer creation changes. |
| opensaas-sh/blog/public/llms.txt | Added a new docs link. |
| opensaas-sh/blog/public/llms-full.txt | Multiple doc updates; links and guidance refreshed. |
| .github/workflows/e2e-tests.yml | Removed obsolete STRIPE_CUSTOMER_PORTAL_URL from CI env. |
| opensaas-sh/app_diff/* | App-diff adjustments mirroring template changes (not core template code). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
opensaas-sh/blog/src/content/docs/guides/payments-integration.mdx
Outdated
Show resolved
Hide resolved
| * Returns a Stripe customer for the given User email, creating a customer if none exist. | ||
| * Implements email uniqueness logic since Stripe doesn't enforce unique emails. | ||
| */ | ||
| export async function ensureStripeCustomer(userEmail: string): Promise<Stripe.Customer> { |
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.
Old name fetchStripeCustomer lied, because it also created a customer if none existed.
| priceId: string; | ||
| customerId: string; | ||
| mode: StripeMode; | ||
| priceId: Stripe.Price['id']; |
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 try to extract types from stripe as much as I can.
If I miss it anywhere it is a mistake.
# Conflicts: # template/app/src/payment/stripe/checkoutUtils.ts
# Conflicts: # opensaas-sh/app_diff/src/analytics/stats.ts.diff # opensaas-sh/app_diff/src/payment/stripe/paymentDetails.ts.diff # opensaas-sh/app_diff/src/payment/stripe/paymentProcessor.ts.diff # template/app/src/analytics/stats.ts # template/app/src/payment/stripe/checkoutUtils.ts # template/app/src/payment/stripe/paymentDetails.ts # template/app/src/payment/stripe/paymentProcessor.ts # template/app/src/payment/stripe/stripeClient.ts # template/app/src/payment/stripe/webhook.ts # template/app/src/payment/stripe/webhookPayload.ts # template/app/src/shared/utils.ts
…e-payment-provider # Conflicts: # opensaas-sh/blog/src/content/docs/blog/2024-12-10-turboreel-os-ai-video-generator-built-with-open-saas.mdx
| "e2e:playwright": "DEBUG=pw:webserver npx playwright test", | ||
| "local:e2e:cleanup-stripe": "PID=$(ps -ef | grep 'stripe listen' | grep -v grep | awk '{print $2}') || true && kill -9 $PID || true", | ||
| "local:e2e:playwright:ui": "npx playwright test --ui", | ||
| "local:e2e:playwright:ui": "SKIP_EMAIL_VERIFICATION_IN_DEV=true npx playwright test --ui", |
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.
Explore: how come we don't set this by default inside of wasp-app-runner if we depend on it?
Why did I add this? Did I do something wrong?
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.
The idea of wasp-app-runner is to run the app and not concern itself with env variables. This was suggested by @sodic and accepted by me so it's following more the Unix philosophy of simple tools doing a single job. That's why the wasp-app-runner "only" runs Wasp apps.
| customer_update: { | ||
| address: "auto", | ||
| }: CreateStripeCheckoutSessionParams): Promise<Stripe.Checkout.Session> { | ||
| // WASP_WEB_CLIENT_URL will be set up by Wasp when deploying to production: https://wasp.sh/docs/deploying |
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.
Reminder to create a issue about solving the env vars situation on open-saas.
Use Wass's defineEnvValidationSchema.
Centralize them in a single place.
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.
this one already exists here #483
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.
@FranjoMindek you can use the env object here instead of going for process.env. The env object has the http://localhost:3000/ as the default value: https://wasp.sh/docs/project/env-vars#server-env-vars
I just realized the docs don't say this:

Made an issue for this: wasp-lang/wasp#3294
infomiho
left a comment
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.
Good stuff @FranjoMindek
I think there is one "big" question to disucss - do we keep the Zod schemas for parsing the payload in the name of uniformity?
Testing it locally
Running it locally with the opensaas.sh got me this error when running wasp db migrate-dev, maybe there is a migration missing?
[ Wasp ] src/payment/stripe/paymentProcessor.ts(64,9): error TS2353: Object literal may only specify known properties, and 'paymentProcessorUserId' does not exist in type 'UserSelect<DefaultArgs>'.
[ Wasp ] src/payment/stripe/paymentProcessor.ts(68,15): error TS2339: Property 'paymentProcessorUserId' does not exist on type '{ id: string; createdAt: Date; email: string | null; username: string | null; isAdmin: boolean; isMockUser: boolean; stripeId: string | null; subscriptionStatus: string | null; subscriptionPlan: string | null; datePaid: Date | null; credits: number; }'.
[ Wasp ] src/payment/stripe/paymentProcessor.ts(73,22): error TS2339: Property 'paymentProcessorUserId' does not exist on type '{ id: string; createdAt: Date; email: string | null; username: string | null; isAdmin: boolean; isMockUser: boolean; stripeId: string | null; subscriptionStatus: string | null; subscriptionPlan: string | null; datePaid: Date | null; credits: number; }'.
opensaas-sh/blog/src/content/docs/guides/payments-integration.mdx
Outdated
Show resolved
Hide resolved
opensaas-sh/blog/src/content/docs/guides/payments-integration.mdx
Outdated
Show resolved
Hide resolved
opensaas-sh/blog/src/content/docs/guides/payments-integration.mdx
Outdated
Show resolved
Hide resolved
| customer_update: { | ||
| address: "auto", | ||
| }: CreateStripeCheckoutSessionParams): Promise<Stripe.Checkout.Session> { | ||
| // WASP_WEB_CLIENT_URL will be set up by Wasp when deploying to production: https://wasp.sh/docs/deploying |
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.
@FranjoMindek you can use the env object here instead of going for process.env. The env object has the http://localhost:3000/ as the default value: https://wasp.sh/docs/project/env-vars#server-env-vars
I just realized the docs don't say this:

Made an issue for this: wasp-lang/wasp#3294
| } | ||
| const priceId = invoiceLineItems[0].pricing?.price_details?.price; | ||
| if (!priceId) { | ||
| throw new Error("Unable to extract price id from items"); |
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.
Nipick: Earlier we threw HttpError in this fn - do we want to use it for this throw as well?
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 researched a bit and HttpError is Wasp error which we use when we want to provide additional data to the web client. Webhook events are not relayed to the web client in any way. So I would maybe remove all notion of HttpError as not to confuse people with it.
Does that make sense to you?
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.
when we want to provide additional data to the web client
There is one more use case, if you check how it's handled in our server code:
if (err instanceof HttpError) {
return res.status(err.statusCode).json({ message: err.message, data: err.data })
}You'll notice it gives you control over the error code as well on the server. Also, it enables you to send some message in the error response vs. just 500 https://github.com/wasp-lang/wasp/blob/main/waspc/data/Generator/templates/server/src/app.js#L16
If you feel like we shouldn't send any extra info (status code, message) that's also fine 👍
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 guess that would be valid for self-documentation reasons.
Stripe only recognizes if its 2XX status code or anything else.
400 and 500 make no difference to it.
I'm fine with just Error here, if you are also, you can resolve, otherwise comment.
Co-authored-by: Mihovil Ilakovac <[email protected]>
Co-authored-by: Mihovil Ilakovac <[email protected]>
Co-authored-by: Mihovil Ilakovac <[email protected]>
…-lang/open-saas into franjo/refactor-stripe-payment-provider
It seems like I need to update the edit: Should be fixed now. |
…e-payment-provider
infomiho
left a comment
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.
Left some more comments
| @@ -1,6 +1,6 @@ | |||
| --- template/app/src/client/Main.css | |||
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.
How come we have these changes in this PR?
Should we maybe do a separate PR that only updates the diffs?
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.
Yep, this often happens.
We should have a CI workflow for this.
| } | ||
| const priceId = invoiceLineItems[0].pricing?.price_details?.price; | ||
| if (!priceId) { | ||
| throw new Error("Unable to extract price id from items"); |
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.
when we want to provide additional data to the web client
There is one more use case, if you check how it's handled in our server code:
if (err instanceof HttpError) {
return res.status(err.statusCode).json({ message: err.message, data: err.data })
}You'll notice it gives you control over the error code as well on the server. Also, it enables you to send some message in the error response vs. just 500 https://github.com/wasp-lang/wasp/blob/main/waspc/data/Generator/templates/server/src/app.js#L16
If you feel like we shouldn't send any extra info (status code, message) that's also fine 👍
| if (process.env.NODE_ENV === "development") { | ||
| console.info("Unhandled Stripe webhook event in development: ", error); | ||
| } else if (process.env.NODE_ENV === "production") { | ||
| console.error("Unhandled Stripe webhook event in production: ", error); |
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'm afraid this might go unnoticed in the logs. But I guess, if you search for the event name - it will appear. How does this error look like in the logs now?
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.
| function getOpenSaasSubscriptionStatus( | ||
| subscription: Stripe.Subscription, | ||
| ): SubscriptionStatus | undefined { | ||
| let subscriptionStatus: SubscriptionStatus | undefined; |
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.
Why not return right away vs. using a let and returning at the end?
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 just extracted the logic out, didn't improve it, my bad.

Description
Fixes #487
The PR idea was to research
stripepayment provider to see If we could optimize our currentstripeintegration.For implementation, my main idea was to simplify webhook handling logic and reduce the number of events we have to handle. Secondary idea was to make it easier for users to work with our
stripeintegration.The simplification is done through:
Stripe.Invoicefor one-time payments (this is a opt-in setting).invoice.paidevent. So we can stop handlingcheckout.session.completedevent, which we handled to update ONLY one-time payments related data.stripecustomer portalstripecustomer postal to be generated by API on per-custmer basis rather than using public customer portal linklemon squeezy, and it makes sense for already logged-in users (you are automatically logged in into your payment provider customer profile, instead of having to login via magic link)webhookPayload.tswhich included payload parsing schema. This is because we already verify the payload viastripe's signature (constructEventfunc), and the type system allows us to navigate the data.Other changes:
try/catchblocksconsole.log(error); throw error;situations