Cancel Stripe subscriptions on account deletion #194
Conversation
WalkthroughThe pull request adds a Stripe property to the application's type declarations and integrates Stripe initialization into the server hooks. The middleware now initializes a Stripe client with the provided API key, making it available in the request context. Additionally, the account deletion endpoint is updated to retrieve the user's Stripe customer details, list active subscriptions, and cancel them before account deletion. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Middleware
participant D as DeleteAccount Handler
participant DB as Database
participant S as Stripe API
C->>M: Send account deletion request
M->>M: Initialize Supabase, Auth, and Stripe client
M->>D: Forward request with attached Stripe client
D->>DB: Query for Stripe customer ID
DB-->>D: Return customer ID
D->>S: Request list of active subscriptions
S-->>D: Return subscriptions list
D->>S: Cancel active subscriptions (in parallel)
D-->>C: Return deletion status
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/app.d.ts (1)
19-19: Type declaration looks good but possibly needs nullability.The stripe property is currently declared as non-nullable, which means TypeScript will expect it to always be defined. Consider whether this property should be optional or nullable if there are cases where the Stripe instance might not be initialized.
- stripe: Stripe + stripe: Stripe | nullAlternatively, if you're certain the Stripe instance will always be available when needed, the current non-nullable type is appropriate.
src/hooks.server.ts (1)
101-106: Stripe initialization looks good with a consideration.The implementation initializes the Stripe client with a specific API version. This follows best practices to avoid unexpected breaking changes when Stripe updates their API.
However, consider extracting the API version to a configuration constant rather than hardcoding it, which would make future updates easier.
+const STRIPE_API_VERSION = "2023-08-16" + const stripe: Handle = async ({ event, resolve }) => { event.locals.stripe = new Stripe(PRIVATE_STRIPE_API_KEY, { - apiVersion: "2023-08-16", + apiVersion: STRIPE_API_VERSION, }) return resolve(event) }src/routes/(admin)/account/api/+page.server.ts (1)
229-236: Optimize the Promise handling.The current implementation creates a Promise variable and then immediately uses Promise.all with a single-element array, which adds unnecessary complexity.
- const subscriptionsPromise = stripe.subscriptions.list({ - customer: customer.stripe_customer_id, - limit: 100, - }) - - const [{ data: subscriptions }] = await Promise.all([ - subscriptionsPromise, - ]) + const { data: subscriptions } = await stripe.subscriptions.list({ + customer: customer.stripe_customer_id, + limit: 100, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.d.ts(2 hunks)src/hooks.server.ts(3 hunks)src/routes/(admin)/account/api/+page.server.ts(3 hunks)
🔇 Additional comments (8)
src/app.d.ts (1)
2-2: Good addition of Stripe type import.This is necessary to properly type the Stripe instance in the Locals interface.
src/hooks.server.ts (3)
2-5: Clean import reorganization.Properly structured imports with the Stripe API key.
14-14: Correct Stripe import.The import is properly added for the Stripe package.
108-108: Sequence configuration is good.The handle sequence is properly updated to include the new stripe middleware.
src/routes/(admin)/account/api/+page.server.ts (4)
183-183: Properly updated function parameter.The stripe instance is now correctly included in the destructured locals parameter.
212-224: Good retrieval of Stripe customer ID.The code properly fetches the user's Stripe customer ID from the database and handles potential errors.
242-246: Proper implementation of cancellation with Promise.all.Using Promise.all for parallel cancellation of subscriptions is efficient and correctly implemented.
238-240:✅ Verification successful
Fix subscription status typo.
The status "trailing" is incorrect, it should be "trialing" according to Stripe's API documentation.
const currentSubscriptions = subscriptions.filter((sub) => - ["active", "trailing", "past_due"].includes(sub.status), + ["active", "trialing", "past_due"].includes(sub.status), )
🌐 Web query:
What are the valid subscription statuses in Stripe API?💡 Result:
Stripe subscriptions can have one of seven possible statuses:
incomplete,incomplete_expired,trialing,active,past_due,canceled, orunpaid[1][4]. Here's a breakdown of each status:
incomplete: Occurs when the first payment attempt fails during subscription creation. This status applies only to the initial payment[1][4].incomplete_expired: Automatically set if a subscription remainsincompletefor 23 hours without successful payment[1].trialing: Active during a free trial period before the first full billing cycle[1][4].active: The subscription is in good standing, with payments successfully processed[1][4].past_due: Triggered when a recurring payment fails after the initial subscription creation[1][4].canceled: The subscription has been terminated and cannot be reactivated[1][4].unpaid: Similar tocanceled, but leaves invoices open for potential future payment if the customer updates their payment method[1][4].The status transitions depend on payment success/failure and retry settings configured in the Stripe Dashboard[4]. For example, failed payments typically move subscriptions to
past_due, while prolonged failures may transition them tocanceledorunpaidbased on recovery rules[4][9].Citations:
- 1: https://mrcoles.com/stripe-api-subscription-status/
- 2: https://docs.stripe.com/api
- 3: https://forum.bubble.io/t/stripe-how-to-validate-if-subscription-is-active/164989
- 4: https://docs.stripe.com/billing/subscriptions/overview
- 5: https://forum.bubble.io/t/stripe-subscription-status-api/251952
- 6: https://docs.stripe.com/api/subscriptions/list
- 7: https://docs.stripe.com/api/subscriptions/update
- 8: https://docs.stripe.com/api/subscriptions/retrieve
- 9: https://docs.stripe.com/api/subscriptions/object
- 10: https://docs.stripe.com/api/subscriptions
Critical: Replace invalid subscription status "trailing" with "trialing".
According to Stripe's API, the valid status is"trialing", not"trailing". Please update the code accordingly:
- File:
src/routes/(admin)/account/api/+page.server.ts- Lines: 238-240
const currentSubscriptions = subscriptions.filter((sub) => - ["active", "trailing", "past_due"].includes(sub.status), + ["active", "trialing", "past_due"].includes(sub.status), )
There was a problem hiding this comment.
Thanks for the PR!
There's 1 issue we'd definitely need to fix (returning plaintext password).
I'm also torn on approach: I'm thinking we just show an error that says something like "You have an active subscription - cancel it from /billing before deleting your account"? It gives the user an extra chance to realize the impact of the change (paid services are about to stop working and there's no way of turning them back on if this is something mission critical). I could see some apps wanting this approach, and others wanting the warning, but the warning seems safer for a boilerplate/template.
As soon as it's canceled (even if still active until end of billing cycle) we should allow deleting the account.
| console.error("Error getting stripe customer ID.", cusError) | ||
| return fail(500, { | ||
| errorMessage: "Unknown error. If this persists please contact us.", | ||
| currentPassword, |
There was a problem hiding this comment.
definitely shouldn't be returning their password in an error message in plaintext
There was a problem hiding this comment.
wow 🥶 thanks for noticing that!
|
You're right it may be problematic that the user may not realize that he'll no longer be able to use the services. To solve for this, we could add a prompt that asks the user to confirm they want to cancel their subscriptions. On the other hand, if we want to take such cases into account, maybe the fully correct solution would be to just cancel the subscriptions and then "schedule" the full account deletion only after their subscriptions run out. Or ask them to come back and retry the deletion after their paid period. I'm not sure whether the above would be what most people deleting their account would expect. 🤔 |
|
@kizivat Let's just keep it simple? How about: if you click delete with an active subscription, show a message "You have an active subscription. If you want to delete your account, cancel it first." with [Cancel] and [View Subscription] buttons? Saves a lot of the complexity and is clear for the user. Keep the server side check they don't have an active sub, but that's it. |
2 participants
fixes #187
I had this implemented in my fork and noticed this issue in in your repo, so I wanted to chip in.
I'm unsure about storing the
Stripeinstance inevent.localsinhook.server.tsbut it seems to me as best approach, which could be accommodated in other parts of the app too, but if you can see some issues with it, I would love your input on it and I can recreate the instance directly in the client module.Also, what could be up for discussion is whether or not the whole account deletion shouldn't go through if the subscription cancellation is not resolved successfully.
I decided to fail the deletion just so the user had a chance to try it again, ideally by first cancelling in the Stripe's Billing Portal.
Summary by CodeRabbit