Skip to content

[usage] Proto changes Stripe RPC #13951

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

Merged
merged 1 commit into from
Oct 18, 2022
Merged

[usage] Proto changes Stripe RPC #13951

merged 1 commit into from
Oct 18, 2022

Conversation

laushinka
Copy link
Contributor

@laushinka laushinka commented Oct 18, 2022

Description

First step in moving Stripe handling from server to the usage component.
The methods added to the Billing proto: CreateStripeSubscription and FindUncancelledSubscriptionByAttributionId.

Related Issue(s)

Relates #13198

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

I believe we can further simplify there.

From the server component, I want to be able to call just one RPC:

subscription = await billingService.CreateStripeSubscription(params)

The rest are not really needed as they are inherently part of the CreateSubscriptionCall

@laushinka laushinka force-pushed the lau/stripe-proto-2-13198 branch from 0b2c8d3 to c5a84fb Compare October 18, 2022 12:22
@laushinka laushinka requested a review from easyCZ October 18, 2022 12:22
@laushinka laushinka force-pushed the lau/stripe-proto-2-13198 branch from c5a84fb to 7df87a7 Compare October 18, 2022 12:47

// GetStripeSubscription without passing in the `status` will return uncancelled subscriptions
// Stripe returns a list which may include more than one, but we will throw an error in that case
rpc GetStripeSubscription(GetStripeSubscriptionRequest) returns (GetStripeSubscriptionResponse) {};
Copy link
Contributor Author

@laushinka laushinka Oct 18, 2022

Choose a reason for hiding this comment

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

I read through the docs again and turns out Stripe accepts a status field with 7 different values. When none are passed, it returns uncancelled ones.
It also returns a list, but in the server I see that we only want one, and throw an error if there is more. Hence why I kept with not naming it ListStripeSubscriptions.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, we should continue to enforce those semantics then

@laushinka laushinka force-pushed the lau/stripe-proto-2-13198 branch from 7df87a7 to 519b18d Compare October 18, 2022 13:16
Comment on lines +88 to +91
message CreateStripeSubscriptionResponse {
StripeCustomer customer = 1;
v1.CostCenter cost_center = 2;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd have expected to get a StripeSubscription in the response. What's the reasoning we're not returning one? Do we not need it?

We can add it later as well.

@roboquat roboquat merged commit e61ab5b into main Oct 18, 2022
@roboquat roboquat deleted the lau/stripe-proto-2-13198 branch October 18, 2022 14:31
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XL team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants