-
Notifications
You must be signed in to change notification settings - Fork 572
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 account debits #1071
Conversation
* A Charge can be created with an Account id as the source. * Deserialization of the Account object will work even though the API only returns an id and an object. * Account debit as a transfer already works as `destination` is a string and can not be expanded.
@@ -0,0 +1,43 @@ | |||
using Machine.Specifications; |
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 went with the Stripe.net.Tests
because that's where all the others are for that specific deserialization use-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.
Ack. Not ideal, but understandable :)
@@ -15,6 +16,7 @@ public class Source : StripeEntityWithId | |||
{ | |||
public SourceType Type { get; set; } | |||
|
|||
public StripeAccount Account { get; set; } |
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.
It seems easier to use StripeAccount
here even though the API only returns this:
{
id: "acct_XXXXX",
object: "account"
}
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.
👍
{ | ||
Amount = 100, | ||
Currency = "usd", | ||
SourceTokenOrExistingSourceId = _stripeAccount.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.
That parameter name is really bad. I wonder if we should go back to just Source
once and for all to match the API. Likely out of scope for this change
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.
==
source.Type = SourceType.Account; | ||
source.Account = Mapper<StripeAccount>.MapFromJson(incoming.ToString()); | ||
} | ||
|
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.
So, I wanted to do an if/else if/ else if/ ... / else
because it's faster but seems there's only one in the entire lib so wasn't sure if there was a reason.
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 don't think there's any specific reason -- these cases are all mutually exclusive so using else if
would be fine.
cc @dawn-stripe for awareness |
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.
Requested an additional test, but otherwise lgtm
@@ -0,0 +1,43 @@ | |||
using Machine.Specifications; |
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.
Ack. Not ideal, but understandable :)
{ | ||
Amount = 100, | ||
Currency = "usd", | ||
SourceTokenOrExistingSourceId = _stripeAccount.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.
==
@@ -15,6 +16,7 @@ public class Source : StripeEntityWithId | |||
{ | |||
public SourceType Type { get; set; } | |||
|
|||
public StripeAccount Account { get; set; } |
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.
👍
source.Type = SourceType.Account; | ||
source.Account = Mapper<StripeAccount>.MapFromJson(incoming.ToString()); | ||
} | ||
|
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 don't think there's any specific reason -- these cases are all mutually exclusive so using else if
would be fine.
|
||
It should_have_the_right_id = () => | ||
_stripeCharge.Source.Id.ShouldEqual(_stripeAccount.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.
Do you mind adding a test for _stripeCharge.Source.Account.Id
, just to make sure the partial account object gets deserialized properly?
@ob-stripe PTAL |
lgtm! |
Deferring to you to merge/release |
destination
is a string and can not be expanded.r? @ob-stripe