-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[usage] Implement CreateStripeSubscription #14044
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
7c293ee
to
c7c4cec
Compare
started the job as gitpod-build-lau-createstripesub-impl.9 because the annotations in the pull request description changed |
5ba024f
to
a774cce
Compare
Pipeline has been acting weird. /werft run 👍 started the job as gitpod-build-lau-createstripesub-impl.24 |
132c297
to
f4acfbf
Compare
Moving this back to draft, we need to clean up a bit and rebase on #14124 before we can land this. |
8271404
to
e482b76
Compare
66b8ad5
to
7359f0b
Compare
Ready for the next round of review, but holding to comment out this test line. /hold |
6106a43
to
f979de3
Compare
I can understand and will keep that in mind for next tasks, but for this one I would like to have a separate feature flag PR[1] to avoid adding more things into this already big PR which has taken quite some time. |
f979de3
to
b7a62fe
Compare
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.
Looking great, almost there.
priceIdentifier := getPriceIdentifier(attributionID, stripeCustomer, s) | ||
if priceIdentifier == "" { | ||
return nil, status.Errorf(codes.InvalidArgument, "Invalid currency %s for customer ID %s", stripeCustomer.Metadata["preferredCurrency"], stripeCustomer.ID) | ||
} |
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.
Because here you only check the empty string, and return an error. You can increase cohesion of this handling by pushing that error into the getPriceIdentifier
. It would look like this:
priceIdentifier := getPriceIdentifier(attributionID, stripeCustomer, s) | |
if priceIdentifier == "" { | |
return nil, status.Errorf(codes.InvalidArgument, "Invalid currency %s for customer ID %s", stripeCustomer.Metadata["preferredCurrency"], stripeCustomer.ID) | |
} | |
priceID, err := getPriceIdentifier(attributionID, stripeCustomer, s) | |
if err != nil { | |
// note we're not wrapping the error, as the `getPriceIdentifier` can do that for us, with more context on the actual failure. | |
return nil, err | |
} |
The getPriceIdentifier
then becomes:
func getPriceIdentifier(attributionID db.AttributionID, stripeCustomer *stripe_api.Customer, s *BillingService) (string, error) {
preferredCurrency := stripeCustomer.Metadata["preferredCurrency"]
if stripeCustomer.Metadata["preferredCurrency"] == "" {
log.
WithField("stripe_customer_id", stripeCustomer.ID).
Warn("No preferred currency set. Defaulting to USD")
}
entity, _ := attributionID.Values()
switch entity {
case db.AttributionEntity_User:
switch preferredCurrency {
case "EUR":
return s.stripePrices.IndividualUsagePriceIDs.EUR, nil
default:
return s.stripePrices.IndividualUsagePriceIDs.USD, nil
}
case AttributionEntity_Team:
switch preferredCurrency {
case "EUR":
return s.stripePrices.TeamUsagePriceIDs.EUR, nil
default:
return s.stripePrices.TeamUsagePriceIDs.USD, nil
}
default:
return "", status.Errorf(codes.InvalidArgument, "Invalid currency %s for customer ID %s", stripeCustomer.Metadata["preferredCurrency"], stripeCustomer.ID)
}
}
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 this is much better, and good point about returning the error here instead.
70cd0b7
to
f6d0b91
Compare
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.
Nice work @laushinka.
Thanks a lot, @easyCZ! Will squash and remove hold. |
f6d0b91
to
cdb2d9e
Compare
Description
Implements CreateStripeSubscription.
Follow-up: Plug it into server with a feature flag.
Related Issue(s)
Depends on #14124
Fixes #13198
How to test
Gitpod
in it (make sure to use the capital 'G').Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide