Add support for PaymentMethod #806

remi-stripe · 2019-03-06T00:37:11Z

cc @stripe/api-libraries

mickjermsurawong-stripe · 2019-03-06T22:54:49Z

charge.go

+
+// PaymentMethodDetailsCardWallet represents the details of the card wallet if this Card
+// PaymentMethod is part of a card wallet.
+type PaymentMethodDetailsCardWallet struct {


@remi-stripe should this be prefixed with ChargeXXX like other structs too?

Yes it should, nice catch.

garrett-stripe · 2019-03-07T04:50:24Z

paymentmethod.go

+// PaymentMethodTransferDataParams is the set of parameters allowed for the transfer_data hash.
+type PaymentMethodTransferDataParams struct {
+	Destination *string `form:"destination"`
+}


I think this is unintentional—this struct doesn't appear to be used anywhere.

ah yes bad copy paste, fixed (won't push while you review)

remi-stripe · 2019-03-14T05:09:53Z

paymentintent.go

@@ -114,14 +114,16 @@ type PaymentIntentListParams struct {

 // PaymentIntentLastPaymentError represents the last error happening on a payment intent.
 type PaymentIntentLastPaymentError struct {


Technically, this should be Error right? Is it worth changing this and releasing as a major version to be cleaner?

remi-stripe · 2019-03-16T04:46:49Z

Okay, this should now be feature complete and ready for review.

There is one test failing, TestTokenNew_SharedCustomerBankAccount and it's because of a change in openapi which does not expect bank_account to be a string even though it can be. cc @mickjermsurawong-stripe as this is likely due to a change in our codebase for stripe-java autogeneration.

r? @brandur-stripe

mickjermsurawong-stripe

Since I reviewed stripe-java against the autogen for schema correctness, it might be easier for me to review stripe-go here too. I reviewed both resource and params here. The params look good to me.

mickjermsurawong-stripe · 2019-03-18T04:25:41Z

charge.go

+
+// ChargePaymentMethodDetailsBancontact represents details about the Bancontact PaymentMethod.
+type ChargePaymentMethodDetailsBancontact struct {
+	AccountHolderType BankAccountAccountHolderType `json:"account_holder_type"`


whoops i think we accidentally duplicated that from AchDebit here.
Bancontact should have bank_code, bank_name, bic.. and

mickjermsurawong-stripe · 2019-03-18T04:32:37Z

charge.go

+
+// ChargePaymentMethodDetailsEps represents details about the EPS PaymentMethod.
+type ChargePaymentMethodDetailsEps struct {
+	Reference    string `json:"reference"`


I think we no longer want reference?
It's not in apiRefs anymore: /docs/api/charges/object#charge_object-payment_method_details-eps

mickjermsurawong-stripe · 2019-03-18T04:34:51Z

charge.go

+	SepaDebit         *ChargePaymentMethodDetailsSepaDebit         `json:"sepa_debit"`
+	Sofort            *ChargePaymentMethodDetailsSofort            `json:"sofort"`
+	StripeAccount     *ChargePaymentMethodDetailsStripeAccount     `json:"stripe_account"`
+	Wechat            *ChargePaymentMethodDetailsWechat            `json:"wechat"`


ah we might be missing type field here

mickjermsurawong-stripe · 2019-03-18T04:42:57Z

paymentmethod.go

+// is part of a card wallet.
+type PaymentMethodCardWallet struct {
+	DynamicLast4 string                      `json:"dynamic_last4"`
+	Type         PaymentMethodCardWalletType `json:"type"`


We miss non-empty visa_checkout, master_pass structs here.
docs/api/payment_methods/object#payment_method_object-card-wallet-masterpass
and
docs/api/payment_methods/object#payment_method_object-card-wallet-visa_checkout
Do we also want to include the empty structs like google_pay and samsung_pay like in java and dotnet?

remi-stripe · 2019-03-18T17:11:05Z

@mickjermsurawong-stripe Thanks a lot for the thorough review. I can't believe after all the back and forth I did on those PRs I still missed the wallet fields on Charge. I fixed everything you found I think. PTAL!

mickjermsurawong-stripe · 2019-03-18T17:44:57Z

thanks remi for the fix!
LGTM

mickjermsurawong-stripe · 2019-03-18T18:01:08Z

ah the test is failing on /v1/tokens as it is trying to create bank account token with a token.
My change on typed param work requires only the bank_account object instead of hash.
But the failing test here is a valid use case we want to support? I can add this back to support string token as well. @remi-stripe

func TestTokenNew_SharedCustomerBankAccount(t *testing.T) {
	params := &stripe.TokenParams{
		BankAccount: &stripe.BankAccountParams{
			ID: stripe.String("ba_123"),
		},
		Customer: stripe.String("cus_123"),
	}
	params.SetStripeAccount("acct_123")
	token, err := New(params)
	assert.Nil(t, err)
	assert.NotNil(t, token)
}

remi-stripe · 2019-03-18T19:17:37Z

You can ignore that failing test, it's being fixed in another PR in parallel I just didn't want to duplicate the work

brandur-stripe

LGTM as well Remi.

brandur-stripe · 2019-03-18T19:46:33Z

paymentmethod.go

+	PaymentMethodCardBrandMastercard PaymentMethodCardBrand = "mastercard"
+	PaymentMethodCardBrandUnionpay   PaymentMethodCardBrand = "unionpay"
+	PaymentMethodCardBrandVisa       PaymentMethodCardBrand = "visa"
+	PaymentMethodCardBrandUnknown    PaymentMethodCardBrand = "unknown"


Minor alphabetization problem here: unknown should be above visa.

Nice catch, fixed

remi-stripe · 2019-03-19T01:03:52Z

I had to remove a test because we don't support bank_account: "ba_123" when creating tokens anymore. cc @mickjermsurawong-stripe

mickjermsurawong-stripe · 2019-03-19T01:13:31Z

LGTM

remi-stripe force-pushed the remi-add-payment-method branch from 125b389 to 6595643 Compare March 6, 2019 20:57

mickjermsurawong-stripe reviewed Mar 6, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-method branch from 6595643 to 4820f40 Compare March 6, 2019 23:19

garrett-stripe reviewed Mar 7, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-method branch 4 times, most recently from 02a083e to 00f558e Compare March 14, 2019 04:45

remi-stripe force-pushed the remi-add-payment-method branch from 00f558e to 6d9c5f5 Compare March 14, 2019 05:08

remi-stripe commented Mar 14, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-method branch 3 times, most recently from faa9ef8 to 6e68eb4 Compare March 16, 2019 04:45

remi-stripe assigned brandur-stripe Mar 16, 2019

remi-stripe force-pushed the remi-add-payment-method branch from 6e68eb4 to 93a53a3 Compare March 17, 2019 23:16

mickjermsurawong-stripe suggested changes Mar 18, 2019

View reviewed changes

stripe-ci assigned remi-stripe and unassigned brandur-stripe Mar 18, 2019

remi-stripe force-pushed the remi-add-payment-method branch from 93a53a3 to 0a05cef Compare March 18, 2019 17:10

remi-stripe assigned mickjermsurawong-stripe and unassigned remi-stripe Mar 18, 2019

brandur-stripe reviewed Mar 18, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-method branch from 0a05cef to a9ed8f4 Compare March 18, 2019 23:33

Add support for PaymentMethod

5a99811

remi-stripe force-pushed the remi-add-payment-method branch from a9ed8f4 to 5a99811 Compare March 19, 2019 01:02

remi-stripe changed the title ~~[WIP]Add support for PaymentMethod~~ Add support for PaymentMethod Mar 19, 2019

mickjermsurawong-stripe approved these changes Mar 19, 2019

View reviewed changes

stripe-ci added the approved label Mar 19, 2019

stripe-ci assigned remi-stripe and unassigned mickjermsurawong-stripe Mar 19, 2019

remi-stripe merged commit 405d49f into master Mar 19, 2019

remi-stripe deleted the remi-add-payment-method branch March 19, 2019 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for PaymentMethod #806

Add support for PaymentMethod #806

remi-stripe commented Mar 6, 2019

mickjermsurawong-stripe Mar 6, 2019

remi-stripe Mar 6, 2019

garrett-stripe Mar 7, 2019

remi-stripe Mar 7, 2019

remi-stripe Mar 14, 2019

remi-stripe commented Mar 16, 2019

mickjermsurawong-stripe left a comment

mickjermsurawong-stripe Mar 18, 2019

mickjermsurawong-stripe Mar 18, 2019

mickjermsurawong-stripe Mar 18, 2019

mickjermsurawong-stripe Mar 18, 2019

remi-stripe commented Mar 18, 2019

mickjermsurawong-stripe commented Mar 18, 2019

mickjermsurawong-stripe commented Mar 18, 2019

remi-stripe commented Mar 18, 2019

brandur-stripe left a comment

brandur-stripe Mar 18, 2019

remi-stripe Mar 18, 2019

remi-stripe commented Mar 19, 2019

mickjermsurawong-stripe commented Mar 19, 2019

		@@ -114,14 +114,16 @@ type PaymentIntentListParams struct {

		// PaymentIntentLastPaymentError represents the last error happening on a payment intent.
		type PaymentIntentLastPaymentError struct {

Add support for PaymentMethod #806

Add support for PaymentMethod #806

Conversation

remi-stripe commented Mar 6, 2019

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

remi-stripe commented Mar 16, 2019

mickjermsurawong-stripe left a 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

remi-stripe commented Mar 18, 2019

mickjermsurawong-stripe commented Mar 18, 2019

mickjermsurawong-stripe commented Mar 18, 2019

remi-stripe commented Mar 18, 2019

brandur-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remi-stripe commented Mar 19, 2019

mickjermsurawong-stripe commented Mar 19, 2019