-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
update stripe sdk and webhooks to match #427
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
template/app/package.json
Outdated
@@ -24,7 +24,7 @@ | |||
"react-hot-toast": "^2.4.1", | |||
"react-icons": "4.11.0", | |||
"react-router-dom": "^6.26.2", | |||
"stripe": "11.15.0", | |||
"stripe": "^18.1.0", |
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.
according to new stripe update policy, this will avoid breaking changes.
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.
Not sure what this means, can you elaborate?
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.
they only introduce breaking changes on major releases, so we can include minor updates.
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.
fixes #293
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.
ok since there's been a new minor update I was getting the error "Type '"2025-04-30.basil"' is not assignable to type '"2025-05-28.basil"' so I pinned the stripe version to avoid future issues.
// We do this so that we can capture priceId in the payment_intent.succeeded webhook | ||
// and easily confirm the user's payment based on the price id. For subscriptions, we can get the price id | ||
// in the customer.subscription.updated webhook via the line_items field. | ||
payment_intent_data: paymentIntentData, |
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.
we're no longer dealing with payment intents to handle one-time payment products. these are now just getting handled directly in checkout.session.completed.
@@ -32,8 +32,8 @@ export const stripeWebhook: PaymentsWebhook = async (request, response, context) | |||
case 'invoice.paid': | |||
await handleInvoicePaid(data, prismaUserDelegate); | |||
break; | |||
case 'payment_intent.succeeded': | |||
await handlePaymentIntentSucceeded(data, prismaUserDelegate); | |||
case 'customer.subscription.created': |
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.
subscription created events used to be sent in customer.subscription.updated events, but they stopped that , so to handle new subscriptions we need this event.
export async function handleInvoicePaid(invoice: InvoicePaidData, prismaUserDelegate: PrismaClient['user']) { | ||
const userStripeId = invoice.customer; | ||
const datePaid = new Date(invoice.period_start * 1000); | ||
return updateUserStripePaymentDetails({ userStripeId, datePaid }, prismaUserDelegate); | ||
const lineItems = await invoiceLineItemsSchema.parseAsync(invoice.lines); |
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.
Not sure if this is needed, because we can be certain that stripe is going to send us the correct format, right?
I'll take a look tomorrow. |
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.
Hmm, I started reviewing, but got a little confused by the context.
The PR description mentions it closes #229 #198 #293 #412. I went through those issues but couldn't figure out how most of the code in this PR relates to them.
@vincanger is there another issue that this PR addresses? Could you comment on the unexpected code and explain how they relate to those issues, and mention the issue they relate to.
As an example, I understand why the stripe version bump is in this PR, but I don't understand what's happening with the Pending status and the stuff in extractPriceId
.
<br/>- `customer.subscription.updated` | ||
<br/>- `invoice.paid` | ||
<br/>- `payment_intent.succeeded` | ||
4. select the events you want to listen to. These should be the same events you're consuming in your webhook which you can find listed in `src/payment/stripe/webhookPayload.ts`: |
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.
Can we link to the file on Github (no permalink, just link to main).
That way, if the file name becomes outdated, the mistake will be easy to spot.
template/app/package.json
Outdated
@@ -24,7 +24,7 @@ | |||
"react-hot-toast": "^2.4.1", | |||
"react-icons": "4.11.0", | |||
"react-router-dom": "^6.26.2", | |||
"stripe": "11.15.0", | |||
"stripe": "^18.1.0", |
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.
Not sure what this means, can you elaborate?
template/app/src/payment/plans.ts
Outdated
@@ -5,6 +5,7 @@ export enum SubscriptionStatus { | |||
CancelAtPeriodEnd = 'cancel_at_period_end', | |||
Active = 'active', | |||
Deleted = 'deleted', | |||
Pending = 'pending', |
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 added this?
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 added it there as kind of a catch in case they introduce async payment methods or trial periods for their product and forget to update the code. See my other comment below.
@@ -8,5 +8,5 @@ export const stripe = new Stripe(requireNodeEnvVar('STRIPE_API_KEY'), { | |||
// npm package to the API version that matches your Stripe dashboard's one. | |||
// For more details and alternative setups check | |||
// https://docs.stripe.com/api/versioning . | |||
apiVersion: '2022-11-15', | |||
apiVersion: '2025-04-30.basil', |
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.
fixes issue #229
@@ -54,33 +51,14 @@ export async function createStripeCheckoutSession({ | |||
success_url: `${DOMAIN}/checkout?success=true`, | |||
cancel_url: `${DOMAIN}/checkout?canceled=true`, | |||
automatic_tax: { enabled: true }, | |||
allow_promotion_codes: true, |
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.
fixes #412
return updateUserStripePaymentDetails( | ||
{ userStripeId, numOfCreditsPurchased, datePaid: new Date() }, | ||
prismaUserDelegate | ||
); |
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.
due to updates to the webhook events on Stripe's side, we can now process credits-based payments and subscriptions within the same endpoint. If it's a subscription payment, numOfCreditsPurchased
will be undefined and thus won't be updated in the DB. We
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 think your sentence got cut off in editing 😄
const subscriptionStatus: SubscriptionStatus = | ||
subscription.status === SubscriptionStatus.Active | ||
? SubscriptionStatus.Active | ||
: SubscriptionStatus.Pending; |
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 a new subscription is created, it can be a few options on Stripe's side: active
, pending
, trial
, etc. Async payment types, such as SEPA transfers, would trigger the pending
type and have to be dealt with in a checkout.session.async_payment_succeeded
event handler (which we don't include here as most people will just use immediate payments with card). In case they do, the status will be updated with pending
here and then later will be sent as completed
in the async handler. Similarly, if they choose to allow for trial periods on their products, they would have to implement that logic to deal with trials
here.
@sodic ok I commented the code where the relevant issues are solved with the exception of #198 which was actually not addressed explicitly in the code, but after reading the docs while working on this PR I learned that they retry failed webhook events with exponential backoff for a significant period of time and alert you if they're unable to succesfully reach your endpoint. |
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.
We had a call, talked about some stuff.
I left two reminder comments, you'll know what to do :)
export async function handleCustomerSubscriptionCreated( | ||
subscription: SubscriptionCreatedData, |
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.
Let's verify whether this is necessary and if not, let's remove it (parts of it that were left in just in case, probably the pending thing).
Talked about it on a call.
if (session.mode !== 'payment' || session.payment_status !== 'paid') { | ||
return; | ||
} |
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.
We talked about some ways to make this more clear: extracting conditions into variables and/or functions (e.g., isOneTimePayment
and calling the function after what it does (e.g., saveOneTimePayment
vs handleSomething
).
I"m making up names randomly, you'll come up with something more appropriate than I could :)
Description
fixes #229 #198 #293 #412
Contributor Checklist