-
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 managing External Accounts #1072
Conversation
7629b01
to
3c7a82d
Compare
3c7a82d
to
ec60dae
Compare
@@ -1,29 +0,0 @@ | |||
using System.Linq; |
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.
Removed this because I don't think it's useful to verify the exception. Especially because it's an API decision that we will likely lift in the future
} | ||
|
||
[Fact] | ||
public void list_contents_equal() |
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.
merged the list tests to avoid having extra fixtures
@@ -50,7 +50,7 @@ public class StripeAccount : StripeEntityWithId | |||
public string Email { get; set; } | |||
|
|||
[JsonProperty("external_accounts")] | |||
public StripeList<Source> ExternalAccounts { get; set; } | |||
public StripeList<StripeExternalAccount> ExternalAccounts { 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.
I think that this makes it a breaking change. be careful @ob-stripe
|
||
namespace Stripe | ||
{ | ||
public class StripeExternalAccountCardUpdateOptions : INestedOptions |
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 needed an extra class because:
- you can only update some properties
- those are sent as a top level parameter and not under
external_account
|
||
[JsonProperty("external_account")] | ||
public StripeAccountBankAccountOptions ExternalAccountBankAccount { 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.
We don't support card on creation because it's blocked in the API by default. would be trivial to add if needed since there's StripeAccountCardOptions
r? @ob-stripe |
@remi-stripe Is this still a WIP? |
@ob-stripe sorry for the mixup, it's ready to be reviewed |
lgtm overall, though I think we should wait until after the holiday freeze to release this as it's a breaking change. We'll also need to update the API reference to document these new methods. |
Love the "Breaking API Change" label, haha (and this PR is great). Just bumping this now that we're post-holiday. Let me know if you guys need any support on the changes to the reference documentation (or just file a DX ticket — Tim's on run, so it should have eyes on it tout suite). |
This PR adds support for managing External Accounts on Custom accounts.
cc @ob-stripe
Fixes #980.