-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow teams to cancel and renew their usage-based subscription in Stripe #10890
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1904,18 +1904,22 @@ export class GitpodServerEEImpl extends GitpodServerImpl { | |
} | ||
} | ||
|
||
async findStripeCustomerIdForTeam(ctx: TraceContext, teamId: string): Promise<string | undefined> { | ||
const user = this.checkAndBlockUser("findStripeCustomerIdForTeam"); | ||
async findStripeSubscriptionIdForTeam(ctx: TraceContext, teamId: string): Promise<string | undefined> { | ||
const user = this.checkAndBlockUser("findStripeSubscriptionIdForTeam"); | ||
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; | ||
} | ||
const subscription = await this.stripeService.findUncancelledSubscriptionByCustomer(customer.id); | ||
return subscription?.id; | ||
} catch (error) { | ||
log.error(`Failed to get Stripe Customer ID for team '${teamId}'`, error); | ||
log.error(`Failed to get Stripe Subscription ID for team '${teamId}'`, error); | ||
throw new ResponseError( | ||
ErrorCodes.INTERNAL_SERVER_ERROR, | ||
`Failed to get Stripe Customer ID for team '${teamId}'`, | ||
`Failed to get Stripe Subscription ID for team '${teamId}'`, | ||
); | ||
} | ||
} | ||
|
@@ -1931,7 +1935,10 @@ export class GitpodServerEEImpl extends GitpodServerImpl { | |
await this.guardTeamOperation(teamId, "update"); | ||
const team = await this.teamDB.findTeamById(teamId); | ||
try { | ||
const customer = await this.stripeService.createCustomerForTeam(user, team!, setupIntentId); | ||
let customer = await this.stripeService.findCustomerByTeamId(team!.id); | ||
if (!customer) { | ||
customer = await this.stripeService.createCustomerForTeam(user, team!, setupIntentId); | ||
} | ||
Comment on lines
+1938
to
+1941
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems it is possible for 2 parallel requests to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
||
await this.stripeService.createSubscriptionForCustomer(customer.id, currency); | ||
} catch (error) { | ||
log.error(`Failed to subscribe team '${teamId}' to Stripe`, 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.
As in another PR, I think this should be returning NotFound and the client handling not found as No subscription exists.
Uh oh!
There was an error while loading. Please reload this page.
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 suchfindSomething
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.
Uh oh!
There was an error while loading. Please reload this page.
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