-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[UBP] Add findStripeSubscriptionId
method to server
#12876
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
Conversation
Add a general method for finding the stripe subscription id for a given attribution id. Implement the existing `findStripeSubscriptionIdForTeam` method in terms of the new method.
started the job as gitpod-build-af-find-stripe-subscription-id.4 because the annotations in the pull request description changed |
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 adding quite a bit of Stripe interaction to server now. The original intent was to keep as much of it on usage
as possible. Have we lifted that goal?
if (attrId.kind == "team") { | ||
await this.guardTeamOperation(attrId.teamId, "get"); | ||
customer = await this.stripeService.findCustomerByTeamId(attrId.teamId); | ||
} else { |
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.
Perhaps worth being explcit about user
in case we add more types of attribution ids.
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 typescript's type narrowing has our back here - we should get a type error in the else
block if any other types are added to the AttributionId
union:
gitpod/components/gitpod-protocol/src/attribution.ts
Lines 8 to 17 in 7f0062a
export type AttributionTarget = "user" | "team"; | |
export interface UserAttributionId { | |
kind: "user"; | |
userId: string; | |
} | |
export interface TeamAttributionId { | |
kind: "team"; | |
teamId: string; | |
} |
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.
Would it? Here we just do a check against == "team"
and then do an else
for anything else. I think it wouldn't fail to compile in that case.
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.
If I extend the AttributionId
union with a new "foo"
type as below:
export type AttributionId = UserAttributionId | TeamAttributionId | FooAttributionId;
export type AttributionTarget = "user" | "team" | "foo";
export interface UserAttributionId {
kind: "user";
userId: string;
}
export interface TeamAttributionId {
kind: "team";
teamId: string;
}
export interface FooAttributionId {
kind: "foo";
someOtherId: string;
}
export namespace AttributionId {
const SEPARATOR = ":";
export function parse(s: string): UserAttributionId | TeamAttributionId | FooAttributionId | undefined {
if (!s) {
return undefined;
}
const parts = s.split(":");
if (parts.length !== 2) {
return undefined;
}
switch (parts[0]) {
case "user":
return { kind: "user", userId: parts[1] };
case "team":
return { kind: "team", teamId: parts[1] };
case "foo":
return { kind: "foo", someOtherId: parts[1] };
default:
return undefined;
}
}
export function render(id: AttributionId): string {
switch (id.kind) {
case "user":
return `user${SEPARATOR}${id.userId}`;
case "team":
return `team${SEPARATOR}${id.teamId}`;
case "foo":
return `foo{SEPARATOR}${id.someOtherId}`;
}
}
}
export type AttributionId = UserAttributionId | TeamAttributionId | FooAttributionId;
export type AttributionTarget = "user" | "team" | "foo";
export interface UserAttributionId {
kind: "user";
userId: string;
}
export interface TeamAttributionId {
kind: "team";
teamId: string;
}
export interface FooAttributionId {
kind: "foo";
someOtherId: string;
}
export namespace AttributionId {
const SEPARATOR = ":";
export function parse(s: string): UserAttributionId | TeamAttributionId | FooAttributionId | undefined {
if (!s) {
return undefined;
}
const parts = s.split(":");
if (parts.length !== 2) {
return undefined;
}
switch (parts[0]) {
case "user":
return { kind: "user", userId: parts[1] };
case "team":
return { kind: "team", teamId: parts[1] };
case "foo":
return { kind: "foo", someOtherId: parts[1] };
default:
return undefined;
}
}
export function render(id: AttributionId): string {
switch (id.kind) {
case "user":
return `user${SEPARATOR}${id.userId}`;
case "team":
return `team${SEPARATOR}${id.teamId}`;
case "foo":
return `foo{SEPARATOR}${id.someOtherId}`;
}
}
}
I get a type error in the else
block here:
customer = await this.stripeService.findCustomerByUserId(attrId.userId); |
because the type of attrId
in the else
block is now UserAttributionId | FooAttributionId
rather than UserAttributionId
and so can't be guaranteed to have a userId
field.
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.
This is quite a neat feature of typescript's discriminated unions:
https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#discriminating-unions
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 I do this in a playground, it compiles
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.
But if you try to do anything with the value of attr
inside your else
block like access userId
, you do get a type error because the type has not been sufficiently narrowed inside the else
(it could still be either a foo
or a user
).
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.
Sure, but that relies on the fact that we wouldn't overload the userId
or another property. If my new type was for a { kind: "superuser", userId: "..."}
then it will compile, but won't work as expected.
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.
yeah, that's true. I assume that if we added a new attribution kind
it would have a non-overlapping shape.
It's a good point - the API to interact with Stripe for the subscription creation flow was originally implemented on |
Description
Add a general method
findStripeSubscriptionId
for finding the Stripe subscription id for a given attribution id.Implement the existing
findStripeSubscriptionIdForTeam
method in terms of the new method.The more general method will be used to look up subscription ids for individual users in subsequent PRs.
Related Issue(s)
Part of #12685
How to test
The flow to sign a team up for UBP still works:
Gitpod
in the name.Release Notes
Documentation
Werft options: