Support source sharing via Stripe Connect #500

remi-stripe · 2017-12-12T14:29:39Z

When using Stripe Connect, you can clone a card/bank account/source form the platform to a connected account. This PR adds support for all of this:

Creating a new token on a connected account passing the customer id in customer and the card id in card.
Creating a new token on a connected account passing the customer id in customer and the bank account id in bank_account.
Creating a new Source on a connected account passing the customer id in customer and the Source id in original_source.

This fixes #499

r? @brandur-stripe

brandur-stripe · 2017-12-12T18:54:16Z

token.go

+	Bank          *BankAccountParams `form:"bank_account"`
+	BankAccountId string             `form:"bank_account"`
+	Card          *CardParams        `form:"card"`
+	CardId        string             `form:"card"`


The addition of BankAccountId and CardId go a little off-road compared to our standard conventions. Generally when referencing an ID, you just do it with the name of the target resource (e.g. passing Plan when creating a subscription).

I wonder if we should add an ID field into the existing BankAccountParams and CardParams instead? Is the reason you didn't try that because the encoding implementation on these two types of params so messed up already?

Yeah it's mostly because I'd have needed a custom encode or something and it felt easier. But I agree it might be better your way. Will try in a bit

remi-stripe · 2017-12-12T20:43:03Z

@brandur-stripe okay I took your approach instead. PTAL

brandur-stripe · 2017-12-12T22:48:52Z

bankaccount.go

@@ -37,6 +37,9 @@ type BankAccountParams struct {
 	// Token is a token referencing an external account like one returned from
 	// Stripe.js.
 	Token string `form:"-"`
+
+	// ID is used when tokenizing a bank account for shared customers
+	ID string `form:"*"`


Do know if the * is required here? If we just need it for AppendToAsSourceOrExternalAccount, it might be worth changing to - to indicate that it's never needed elsewhere.

@brahn-stripe with - the field seems ignored. Not sure if I'm making another mistake but it's not sent to stripe-mock in that case

sorry that was for @brandur-stripe

Ah okay, this was more of a question I guess. If you look at BankAccountParams, it's basically serialized in two separate ways:

When it's used for bankaccount or token operations, the default form implementation is used.

In certain cases (e.g. recipients), we call the special AppendToAsSourceOrExternalAccount function.

I just want to make sure that we're implementing the minimal logic that we need so that we don't make this anymore complicated. If we need the new ID field for just (1) above, then we should remove the changes in AppendToAsSourceOrExternalAccount. If we need it for just (2) (I don't think this is the case because you implemented a new test for token), then we should have just form:"-" to indicate that this field is never needed by form. If we need ID to be encoded for both (1) and (2), then we should leave this as is.

Can you just confirm that the latter is the case?

@brandur-stripe My understanding was that we call AppendToAsSourceOrExternalAccount even for Token but I see now that this is wrong and I did not need to add the if inside the custom append. I will fix

brandur-stripe · 2017-12-12T22:50:22Z

bankaccount.go

@@ -68,6 +71,8 @@ func (a *BankAccountParams) AppendToAsSourceOrExternalAccount(body *form.Values)
 		if a.Default {
 			body.Add("default_for_currency", strconv.FormatBool(a.Default))
 		}
+	} else if len(a.ID) > 0 {
+		body.Add("bank_account", a.ID)


Would you mind adding a quick test in bankaccount_test.go (there should a few for the params in there already) to double-check this behavior? One of the reasons that we had a few sources-related regressions during the form changeover was that their parameters tend to have pretty complicated encoding logic, but didn't really have any good testing around it.

Hmmm isn't the test in token/client_test.go enough? What do you want me to test?

When implementing custom encoders like AppendToAsSourceOrExternalAccount, it's just kind of nice to know that we're encoding as we expect. token/client_test.go is something, but stripe-mock's validations are nowhere near as strict as the real API's, so we can't guarantee that we didn't make an encoding mistake that was allowed through.

There are some encoding tests already in bankaccount_test.go, but I was just thinking something like this:

func TestBankAccountParams_AppendToAsSourceOrExternalAccount(t *testing.T) { // Sets ID { params := &BankAccountParams{ID: "ba_123"} body := &form.Values{} params.AppendToAsSourceOrExternalAccount(body) t.Logf("body = %+v", body) assert.Equal(t, []string{"ba_123"}, body.Get("bank_account")) } ... }

brandur-stripe · 2017-12-12T22:50:49Z

token.go

-	Customer string             `form:"customer"`
+	Params        `form:"*"`
+	Bank          *BankAccountParams `form:"bank_account"`
+	BankAccountId string             `form:"bank_account"`


(I think the intention here was probably to remove BankAccountId as well as CardId.

correct, missed that one

brandur-stripe · 2017-12-12T22:51:12Z

Thanks Remi! Left a couple more comments, but this is looking like it's getting to a very good place.

When using Shared Customers, you can take a card or bank account on the platform and create a new token for it on a connected account by passing its id in card/bank_account along with the customer id.

remi-stripe · 2017-12-12T23:44:52Z

@brandur-stripe Okay I think I did this one properly this time! PTAL

brandur-stripe · 2017-12-13T00:54:31Z

LGTM. Thanks!

stripe-ci assigned brandur-stripe Dec 12, 2017

remi-stripe mentioned this pull request Dec 12, 2017

Direct Charge on Platform Account Customer to Connected Account Not working #499

Closed

brandur-stripe reviewed Dec 12, 2017

View reviewed changes

remi-stripe force-pushed the remi-fix-card-source-sharing branch from cbfc009 to e5fcf93 Compare December 12, 2017 20:42

brandur-stripe reviewed Dec 12, 2017

View reviewed changes

remi-stripe force-pushed the remi-fix-card-source-sharing branch from 10b00c9 to f600e19 Compare December 12, 2017 23:15

remi-stripe added 2 commits December 12, 2017 18:40

Support passing original_source to clone a source to a connected account

d314422

Properly support passing card and bank_account on Token creation

2098ea6

When using Shared Customers, you can take a card or bank account on the platform and create a new token for it on a connected account by passing its id in card/bank_account along with the customer id.

remi-stripe force-pushed the remi-fix-card-source-sharing branch from f600e19 to 2098ea6 Compare December 12, 2017 23:44

brandur-stripe merged commit 5a3d733 into master Dec 13, 2017

brandur-stripe deleted the remi-fix-card-source-sharing branch December 13, 2017 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support source sharing via Stripe Connect #500

Support source sharing via Stripe Connect #500

remi-stripe commented Dec 12, 2017

brandur-stripe Dec 12, 2017

remi-stripe Dec 12, 2017

remi-stripe commented Dec 12, 2017

brandur-stripe Dec 12, 2017

remi-stripe Dec 12, 2017

remi-stripe Dec 12, 2017

brandur-stripe Dec 12, 2017

remi-stripe Dec 12, 2017

brandur-stripe Dec 12, 2017

remi-stripe Dec 12, 2017

brandur-stripe Dec 12, 2017

brandur-stripe Dec 12, 2017

remi-stripe Dec 12, 2017

brandur-stripe commented Dec 12, 2017

remi-stripe commented Dec 12, 2017

brandur-stripe commented Dec 13, 2017

Support source sharing via Stripe Connect #500

Support source sharing via Stripe Connect #500

Conversation

remi-stripe commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remi-stripe commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Dec 12, 2017

remi-stripe commented Dec 12, 2017

brandur-stripe commented Dec 13, 2017