Skip to content

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

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

Conversation

vincanger
Copy link
Collaborator

@vincanger vincanger commented May 13, 2025

Description

fixes #229 #198 #293 #412

Contributor Checklist

Make sure to do the following steps if they are applicable to your PR:

@vincanger vincanger requested a review from sodic May 14, 2025 09:23
@@ -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",
Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixes #293

Copy link
Collaborator Author

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,
Copy link
Collaborator Author

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':
Copy link
Collaborator Author

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);
Copy link
Collaborator Author

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?

@sodic
Copy link
Collaborator

sodic commented May 19, 2025

I'll take a look tomorrow.

Copy link
Collaborator

@sodic sodic left a 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`:
Copy link
Collaborator

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.

@@ -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",
Copy link
Collaborator

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?

@@ -5,6 +5,7 @@ export enum SubscriptionStatus {
CancelAtPeriodEnd = 'cancel_at_period_end',
Active = 'active',
Deleted = 'deleted',
Pending = 'pending',
Copy link
Collaborator

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?

Copy link
Collaborator Author

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',
Copy link
Collaborator Author

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,
Copy link
Collaborator Author

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
);
Copy link
Collaborator Author

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

Copy link
Collaborator

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;
Copy link
Collaborator Author

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.

@vincanger
Copy link
Collaborator Author

@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.

Copy link
Collaborator

@sodic sodic left a 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 :)

Comment on lines +127 to +128
export async function handleCustomerSubscriptionCreated(
subscription: SubscriptionCreatedData,
Copy link
Collaborator

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.

Comment on lines 98 to 100
if (session.mode !== 'payment' || session.payment_status !== 'paid') {
return;
}
Copy link
Collaborator

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 :)

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.

Upgrade Stripe client to use API version 2023-08-16
2 participants