-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow teams to cancel and renew their usage-based subscription in Stripe #10890
Conversation
… subscription in Stripe
/werft run 👍 started the job as gitpod-build-jx-allow-cancel.1 |
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 is the cancellation triggered by the user? I'm assuming it's using the customer portal.
await this.ensureIsUsageBasedFeatureFlagEnabled(user); | ||
await this.guardTeamOperation(teamId, "update"); | ||
try { | ||
const customer = await this.stripeService.findCustomerByTeamId(teamId); | ||
return customer?.id || undefined; | ||
if (!customer?.id) { | ||
return 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.
As in another PR, I think this should be returning NotFound and the client handling not found as No subscription exists.
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.
Until now, I think we've had our API return something | undefined
for such findSomething
methods.
We could change this pattern to introduce 404
errors instead, but then maybe it would make sense to change all these methods?
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.
Up to you. I'd mostly suggest we do this opportunistically on apis we work on as part of features.
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.
Follow-up issue: #10892
let customer = await this.stripeService.findCustomerByTeamId(team!.id); | ||
if (!customer) { | ||
customer = await this.stripeService.createCustomerForTeam(user, team!, setupIntentId); | ||
} |
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.
It seems it is possible for 2 parallel requests to subscribeTeamToStripe
to succeed and create 2 different subscriptions. Is this somehow transactional on the Stripe side?
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.
Thanks for identifying this pre-existing problem! I don't think it can realistically happen, but I'm happy to open a follow-up issue about this if it seems important. 💡
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.
No need to solve in this PR. I would like an issue for this however. In the least, the Billing Controller (when listing subscriptions for a team) need to tell us there were more than 1 for a given TeamID so we can fix it.
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 controller already handles this case - https://github.com/gitpod-io/gitpod/pull/10854/files#diff-1f1047abea4ed15decb31dfdf94d84646a601fbacd5462dcef20f7f8b4de33dbR54 it will ignore any with multiple subscriptions and logs an error.
Thanks @easyCZ for the initial feedback!
Please read the provided test instructions:
|
Just ran through the steps:
Here, the problem is the plan remains |
I think we should return a Typed error from the Subscription lookup by Team when there are more than 1, which prevents you from creating another subscription (on the server side) but also tell you that there are multiple subscriptions and to please reach out to support. |
Good catch! This is #10656, which I'm planning to fix soon. 📝
Did you achieve this by quickly completing the billing setup again, as allowed by #10656? If so, when we fix the UI state, this bug will become much harder to reproduce. However, I do see your point about the race condition / transaction, so I'll open a follow-up issue for this as well. |
Yes. 2 windows. But with the delay for the subscription to be created (on Stripe), this didn't really need special timing either. We don't need to fix the create, just need to communicate the issue better and ask people to reach out. Thanks for creating the issue! |
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.
Works as advertised.v User can unsubscribe and then re-subscribe modulo other related issues discussed.
Thanks for the change!
Description
Allow teams to cancel and renew their usage-based subscription in Stripe.
Before:
After:
Related Issue(s)
Fixes #
How to test
Release Notes
Documentation
Werft options: