Add support for the PaymentIntent resource #1222
Conversation
| fixture.PaymentIntent.Amount.Should().Be(fixture.PaymentIntentCreateOptions.Amount); | ||
| fixture.PaymentIntent.CaptureMethod.Should().Be(fixture.PaymentIntentCreateOptions.CaptureMethod); | ||
| fixture.PaymentIntent.Currency.Should().Be(fixture.PaymentIntentCreateOptions.Currency); | ||
| fixture.PaymentIntent.AllowedSourceTypes.Count.Should().Be(fixture.PaymentIntentCreateOptions.AllowedSourceTypes.Count); |
There was a problem hiding this comment.
I could not, for the life of me, figure out how to compare the 2 lists. I tried a lot of options but none worked/compiled for some reasons so I settled for the count for now.
|
|
||
| namespace Stripe.Infrastructure.Middleware | ||
| { | ||
| internal class PaymentIntentAllowedSourceTypesPlugin : IParserPlugin |
There was a problem hiding this comment.
This one is dirty/sad but it's just temporary because I needed to make it work. We should ship only after @ob-stripe other PR #1218 instead.
| namespace Stripe | ||
| { | ||
| public class StripePaymentIntentConfirmOptions : StripeBaseOptions | ||
| { |
There was a problem hiding this comment.
Those are a bit frustrating, they each support a subset of the other endpoints params. Not sure this was the best approach
| { | ||
| public class StripePaymentIntentTransferDataOptions : INestedOptions | ||
| { | ||
| [JsonProperty("transfer_data[amount]")] |
There was a problem hiding this comment.
I'll never understand why we need to repeat the parent property's parameter name. Haven't tested either as the test suite would require a custom account and all that.
|
|
||
| [JsonProperty("metadata")] | ||
| public Dictionary<string, string> Metadata { get; set; } | ||
|
|
There was a problem hiding this comment.
missing support for next_source_action
There was a problem hiding this comment.
Done, though the code feels really ugly 😢
| @@ -0,0 +1,145 @@ | |||
| using System; | |||
There was a problem hiding this comment.
Can you confirm that the folder structure is the right approach?
|
@remi-stripe Looks like this one also needs to be rebased and fixed. |
|
Yes, we should hold off a bit on that one since the API could be changing in the next few days. I'll rebase once we know the exact/final shape |
remi-stripe
force-pushed
the
remi-add-payment-intents
branch
from
5d03899 to
a07c807
Compare
|
r? @ob-stripe assigning to you for review as I've redone the PR to match the new test suite and future naming. Some users need this asap so it could not wait the next major revision after all. |
| { | ||
| AuthorizeWithUrl, | ||
| None, | ||
| Unknown, |
There was a problem hiding this comment.
Can we have Unknown as the first value in the enum, so that when you initialize a new PaymentIntentSourceActionType, the Type field defaults to Unknown?
There was a problem hiding this comment.
Hmmm sure but that breaks the "alphabetical order" rule :)
There was a problem hiding this comment.
That's fine. I don't think it makes sense for enums anyway -- if we add a new value to an existing enum, we should append it after all existing values, otherwise it's technically a breaking change.
| using Newtonsoft.Json; | ||
|
|
||
| public class PaymentIntentConfirmOptions : StripeBaseOptions | ||
| { |
There was a problem hiding this comment.
According to the OpenAPI spec, this request also has an additional client_secret string parameter.
There was a problem hiding this comment.
That's a client-side property used with the publishable key, I'm not sure this is a useful parameter in the server-side library. Do you still think I should add it? It's not in our docs.
There was a problem hiding this comment.
Ack, let's ignore it for now -- we can always append it later if needed.
remi-stripe
force-pushed
the
remi-add-payment-intents
branch
from
a07c807 to
25492a3
Compare
|
@ob-stripe PTAL |
remi-stripe
changed the title
This PR adds support for the Payment Intent resource.
cc @stripe/api-libraries @michelle-stripe @jenan-stripe
cc @stevene-stripe