Add support for the PaymentMethod resource #1541

remi-stripe · 2019-02-25T01:16:54Z

cc @stripe/api-libraries

src/Stripe.net/Entities/PaymentMethods/PaymentMethodCardWallet.cs

remi-stripe · 2019-03-14T05:17:24Z

src/Stripe.net/Entities/PaymentIntents/PaymentIntentLastPaymentError.cs

@@ -23,6 +23,12 @@ public class PaymentIntentLastPaymentError : StripeEntity
        [JsonProperty("param")]
        public string Param { get; set; }

+        [JsonProperty("payment_intent")]


Should we fix this to be a StripeError instead and release as a major version?

I'd say yes.

(Although maybe just bundle this with the next release for the new API version?)

remi-stripe · 2019-03-16T03:58:35Z

src/StripeTests/Services/BankAccounts/BankAccountServiceTest.cs

-                        { "key", "value" },
-                    },
-                }
+                SourceToken = "btok_123",


openapi is now failing because this can only be a string instead of a hash. It used to work and I think it's related to some recent changes that @mickjermsurawong-stripe did for stripe-java.

Ultimately it's likely okay to just pass a string here, the library is already well-tested in general

Ah sorry, I didn't foresee it would fail current library tests. Yup, it is my work on typed params to make public spec for these params only taking String as an attempt to limit non-PCI compliant users from accidentally passing in raw PAN data. So as I understand, we want to make this enforcement on in the client lib too, so passing String here would be okay.

Just a side note: on live request OpenAPI validation, we skipped validation on these undocumented polymorphic param so we won't see false rate of validation failure. (Additionally, there's no sever side effect because its' OpenAPI)

remi-stripe · 2019-03-16T03:58:46Z

src/StripeTests/Services/ExternalAccounts/ExternalAccountServiceTest.cs

-                    Currency = "usd",
-                    RoutingNumber = "110000000",
-                }
+                ExternalAccountTokenId = "btok_123",


same as above, cc @mickjermsurawong-stripe

Ack, yup it applies to external account to. The only old endpoint that takes full hash is the token creation params that takes full card/bank account

src/Stripe.net/Services/PaymentMethods/PaymentMethodCreateOptions.cs

remi-stripe · 2019-03-16T04:43:36Z

Okay this is now supposed to be feature complete and ready for review. r? @ob-stripe

mickjermsurawong-stripe

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

mickjermsurawong-stripe · 2019-03-18T04:54:27Z

...Stripe.net/Entities/Charges/ChargePaymentMethodDetails/ChargePaymentMethodDetailsAchDebit.cs

+        public string Fingerprint { get; set; }
+
+        [JsonProperty("routing_number")]
+        public string RoutingNumber { get; set; }


nit: last4 should come before routing_number alphabetically?

mickjermsurawong-stripe · 2019-03-18T04:59:18Z

...et/Entities/Charges/ChargePaymentMethodDetails/ChargePaymentMethodDetailsCardThreeDSecure.cs

+    public class ChargePaymentMethodDetailsCardThreeDSecure : StripeEntity
+    {
+        [JsonProperty("supported")]
+        public bool Supported { get; set; }


I think it should be empty class here. The "supported" field is correctly present in PaymentMethodCardThreeDSecureUsage though.

mickjermsurawong-stripe · 2019-03-18T05:02:46Z

src/Stripe.net/Entities/Charges/ChargePaymentMethodDetails/ChargePaymentMethodDetailsIdeal.cs

+        public string Bic { get; set; }
+
+        [JsonProperty("verified_name")]
+        public string VerifiedName { get; set; }


I think we got the schema from Giropay above instead. Ideal should have iban_last4 and bank

mickjermsurawong-stripe · 2019-03-18T05:15:36Z

src/Stripe.net/Services/PaymentMethods/PaymentMethodUpdateOptions.cs

+    public class PaymentMethodUpdateOptions : BaseOptions
+    {
+        [JsonProperty("metadata")]
+        public Dictionary<string, string> Metadata { get; set; }


I compare with autogen params, and we seem to allow updating card and billing details too..
Maybe we don't want this?

Nice catch, forgot to fix that one. cc @ob-stripe Are we okay having a nested option have names such as PaymentMethodCardCreateOptions and PaymentMethodCardUpdateOptions?

I think that's fine. I assume most users rely on Intellisense to discover classes anyway.

remi-stripe · 2019-03-18T17:27:32Z

@mickjermsurawong-stripe Thanks for the thorough reviews and catching multiple mistakes here!

mickjermsurawong-stripe · 2019-03-18T18:06:48Z

Thanks for the fix remi. The schema looks great to me!
I defer to @ob-stripe for the pending issue on the payment method update options.

ob-stripe

Left a couple of comments. Also, don't forget to remove the spec and fixtures files and use the latest stripe-mock before merging to master.

ob-stripe · 2019-03-19T01:30:42Z

src/Stripe.net/Entities/Charges/ChargePaymentMethodDetails.cs

+
+        [JsonProperty("stripe_account")]
+        public ChargePaymentMethodDetailsStripeAccount StripeAccount { get; set; }
+


type is missing.

ob-stripe · 2019-03-19T01:32:49Z

...ipe.net/Entities/Charges/ChargePaymentMethodDetails/ChargePaymentMethodDetailsCardPresent.cs

+        public string Funding { get; set; }
+
+        [JsonProperty("generated_card")]
+        public string GeneratedCard { get; set; }


If this is an ID, this should be GeneratedCardId.

ob-stripe · 2019-03-19T01:37:38Z

src/Stripe.net/Entities/PaymentIntents/PaymentIntentLastPaymentError.cs

@@ -23,6 +23,12 @@ public class PaymentIntentLastPaymentError : StripeEntity
        [JsonProperty("param")]
        public string Param { get; set; }

+        [JsonProperty("payment_intent")]


(Although maybe just bundle this with the next release for the new API version?)

fixed the issues

brandur-stripe reviewed Mar 6, 2019

View reviewed changes

src/Stripe.net/Entities/PaymentMethods/PaymentMethodCardWallet.cs Outdated Show resolved Hide resolved

remi-stripe force-pushed the remi-add-payment-methods branch 4 times, most recently from 841a1c4 to 74d42f9 Compare March 14, 2019 05:16

remi-stripe commented Mar 14, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-methods branch 3 times, most recently from b88030d to f29ab65 Compare March 16, 2019 03:57

remi-stripe commented Mar 16, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-methods branch from f29ab65 to 7d58b4d Compare March 16, 2019 04:11

remi-stripe commented Mar 16, 2019

View reviewed changes

src/Stripe.net/Services/PaymentMethods/PaymentMethodCreateOptions.cs Outdated Show resolved Hide resolved

remi-stripe force-pushed the remi-add-payment-methods branch 2 times, most recently from 46363aa to 3b9910a Compare March 16, 2019 04:43

remi-stripe assigned ob-stripe Mar 16, 2019

remi-stripe force-pushed the remi-add-payment-methods branch 2 times, most recently from 7a72407 to d93329d Compare March 17, 2019 23:18

mickjermsurawong-stripe suggested changes Mar 18, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-methods branch from d93329d to 8936c9a Compare March 18, 2019 17:26

mickjermsurawong-stripe approved these changes Mar 19, 2019

View reviewed changes

ob-stripe previously requested changes Mar 19, 2019

View reviewed changes

remi-stripe force-pushed the remi-add-payment-methods branch 2 times, most recently from ca14631 to 07e4b2b Compare March 19, 2019 01:52

Add support for the PaymentMethod resource

1d8ea30

remi-stripe force-pushed the remi-add-payment-methods branch from 07e4b2b to 1d8ea30 Compare March 19, 2019 02:26

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

remi-stripe merged commit 87ae809 into master Mar 19, 2019

remi-stripe deleted the remi-add-payment-methods branch March 19, 2019 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the PaymentMethod resource #1541

Add support for the PaymentMethod resource #1541

remi-stripe commented Feb 25, 2019

remi-stripe Mar 14, 2019

ob-stripe Mar 19, 2019

ob-stripe Mar 19, 2019

remi-stripe Mar 16, 2019

mickjermsurawong-stripe Mar 16, 2019

remi-stripe Mar 16, 2019

mickjermsurawong-stripe Mar 16, 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 Mar 18, 2019

ob-stripe Mar 19, 2019

remi-stripe commented Mar 18, 2019

mickjermsurawong-stripe commented Mar 18, 2019

ob-stripe left a comment

ob-stripe Mar 19, 2019

ob-stripe Mar 19, 2019

ob-stripe Mar 19, 2019


		[JsonProperty("stripe_account")]
		public ChargePaymentMethodDetailsStripeAccount StripeAccount { get; set; }

Add support for the PaymentMethod resource #1541

Add support for the PaymentMethod resource #1541

Conversation

remi-stripe commented Feb 25, 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

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

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

ob-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