-
Notifications
You must be signed in to change notification settings - Fork 452
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
Add support for the Checkout Session resource #740
Conversation
4c49cb0
to
e59dba0
Compare
e59dba0
to
7bb4688
Compare
Related to stripe/stripe-dotnet#1440 |
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.
Awesome Remi. Left a couple minor comments, but looking great.
ptal @remi-stripe
}) | ||
assert.Nil(t, err) | ||
assert.NotNil(t, session) | ||
} |
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.
Should we try to exercise Get
as well?
// creation on a checkout session. | ||
type CheckoutSessionSubscriptionDataParams struct { | ||
Items *CheckoutSessionSubscriptionDataItemsParams `form:"items"` | ||
TrialEnd *int64 `form:"trial_end"` |
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 might also need its own Params
so that it can get a Metadata
.
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.
Ah yes I forgot to ask you: does it just work? Do we explicitly go through nested Params
the right way?
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.
You'd have to write a test to be 100% sure, but yeah, Params
doesn't do anything special really, so it should work fine even if nested down on a substructure like this.
The one downside of course is that Params
has a bunch of things on it that are not relevant to this struct (e.g. idempotency key, headers, Stripe-Account
...), so another alternative would be just to add Metadata map[string]string
form:"metadata"`` directly. The downside of that of course is that you'd also have to manually add the AddMetadata
helper to the struct if you wanted to support that.
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.
Woot that magically just works. Nice catch! I think it's likely easier and more explicit that way than having to do something custom that users would not expect.
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.
Okay, WFM.
// CheckoutSessionPaymentIntentDataTransferDataParams is the set of parameters allowed for the | ||
// transfer_data hash. | ||
type CheckoutSessionPaymentIntentDataTransferDataParams struct { | ||
Destination *string `form:"destination"` |
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.
Should this also have Amount
?
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 amount
is gated in theory (and there for PI because it was ungated before launch)
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.
Ah, got it. Okay, thanks!
7bb4688
to
ea3749a
Compare
@brandur-stripe Thanks for catching those issues. I just fixed each one. PTAL |
This resource is now ready to be released. The PR has been updated to match the latest spec.
r? @brandur-stripe
cc @stripe/api-libraries