Add STPPaymentIntent, and method to retrieve from the API given a clientSecret #985

danj-stripe · 2018-07-06T01:12:38Z

Summary

Adding support for /v1/payment_intent/:id

Motivation

IOS-792

Testing

Added some tests. This is the first in a series of PRs that'll add full PaymentIntent
support, and this was tested in the sample app too.

danj-stripe · 2018-07-06T01:13:44Z

Not ready to merge yet, still needs implementation of STPPaymentIntent description, and better tests for decoding from JSON response (see FIXMEs). However, it is ready for early reviewing.

…entSecret Still needs implementation of `STPPaymentIntent description`, and better tests for decoding from JSON response

I'm omitting our normal enum to string helper methods, and instead reaching directly into the `allResponseFields` dictionary. I think this is slightly preferable, because `Unknown` enum values will be represented as the actual string value instead, which may help with debugging. Also logging the `shipping` and `nextSourceAction` fields, even though those aren't exposed by the class yet.

…ntIntent

Adding a canceled_at time to the JSON, otherwise the test that the allResponseFields matches the json object fails - because we strip null values out.

danj-stripe · 2018-07-09T16:54:18Z

This is ready for review now. The PaymentIntent.shipping field support has been deferred from the initial release, see IOS-811.

joeydong-stripe · 2018-07-09T20:42:49Z

Stripe/PublicHeaders/STPAPIClient.h

+@interface STPAPIClient (PaymentIntents)
+
+/**
+ Retrieves the PaymentIntent object using the given secret. @see https://stripe.com/docs/api#retrieve_payment_intent


This link isn't working for me. Is it going to be soon?

found it :)

joeydong-stripe · 2018-07-09T20:43:32Z

Stripe/PublicHeaders/STPAPIClient.h

+ @param secret      The client secret of the payment intent to be retrieved. Cannot be nil.
+ @param completion  The callback to run with the returned PaymentIntent object, or an error.
+ */
+- (void)retrievePaymentIntentWithClientSecret:(NSString *)secret


I'm wondering if there's less ambiguous vocabulary we can use so it's not confused with Stripe client secret? Or is it?

looks like the rest of our documentation uses this ambiguous vocabulary so nevermind :)

if we're confident it's not going to change, maybe we can mention it starts with pi_* and assert for that fact before putting it on the network

client_secret is a secret for working with a specific PaymentIntent or Source, and is different from the publishable key (or the secret key that should never be part of the SDK). (I think you figured this out, but commenting for posterity)

I'm still not sure what the right level of client-side protection is, vs encoding specifics about the current state of things.

joeydong-stripe · 2018-07-09T20:50:05Z

Stripe/STPPaymentIntent.m

+    NSNumber *amount = [dict stp_numberForKey:@"amount"];
+    NSString *currency = [dict stp_stringForKey:@"currency"];
+    NSString *rawStatus = [dict stp_stringForKey:@"status"];
+    if (!stripeId || !clientSecret || amount == nil || !currency || !rawStatus || !dict[@"livemode"]) {


can you do !amount here too to be consistent? now I'm questioning the behavior for NSNumber

also do we need to fail if rawStatus or livemode are missing since we have STPPaymentIntentStatusUnknown or the default livemode = YES?

!amount triggers a Faux Pas warning, protecting us against accidentally checking nil/not-nil instead of YES/NO.

I believe we do want to enforce these being required.

joeydong-stripe · 2018-07-09T20:57:18Z

Stripe/PublicHeaders/STPPaymentIntentEnums.h

+    STPPaymentIntentStatusSucceeded,
+
+    /**
+     Indicates the payment must be captured, for STPPaymentIntentCaptureMethodManual


"Indicates the payment must be captured. The captureMethod is also set to STPPaymentIntentCaptureMethodManual."?

I guess what you have is fine, I'm just wondering what's the intention behind having a separate captureMethod field and if we can communicate that reason.

captureMethod might be interesting to the app's business logic even when the status is not .requiresCapture, but yeah, I'm not sure how this is most likely to be used. Primarily just exposing what exists in the API.

joeydong-stripe · 2018-07-09T21:00:02Z

Stripe/STPAPIClient.m

+                                   completion:(STPPaymentIntentCompletionBlock)completion {
+    NSCAssert(secret != nil, @"'secret' is required to retrieve a PaymentIntent");
+    NSCAssert(completion != nil, @"'completion' is required to use the PaymentIntent that is retrieved");
+    NSString *identifier = [STPPaymentIntent idFromClientSecret:secret];


Do we need to check that identifier is not nil here?

I'm not sure.

I didn't want to NSCAssert, because the most likely way this'd be nil is a change on the Stripe server to the client_secret format, and that'd be a really bad experience if someone shipped an app with asserts enabled.

Similarly, we don't have a pattern (that I found) for early exit, instead we seem to pass (potentially) bad data to the server and allow it to generate an appropriate error. So I think consistency is my preferred choice.

The other alternative that comes to mind is constructing a STPInvalidRequestError error here, and populating the user info with useful values.

joeydong-stripe · 2018-07-09T21:05:04Z

Stripe/PublicHeaders/STPBlocks.h

+/**
+ A callback to be run with a PaymentIntent response from the Stripe API.
+
+ @param paymentIntent The Stripe PaymentIntent from the response. Will be nil if an error occurs. @see STPPaymentIntent


do we need to specifically say STPPaymentIntent.h here like you did for StripeError.h? I'm just imagining this from a very unaware perspective.

IDK? I was following the pattern of every other callback here.

I think this is reasonable, because it's saying "see STPPaymentIntent" (the class), vs "see the values in StripeError.h" (the header with a bunch of values).

joeydong-stripe · 2018-07-09T21:07:15Z

Stripe/PublicHeaders/STPAPIClient.h

+#pragma mark Payment Intents
+
+/**
+ Stripe extensions for working with PaymentIntent objects.


Looks like the docs for the other extension interface definitions in this file say "STPAPIClient extensions..."

There's actually a mix of extensions on STPAPIClient and on Stripe, which I didn't realize at first. Great catch, thanks!

joeydong-stripe · 2018-07-09T21:08:27Z

Stripe/PublicHeaders/STPBlocks.h

+ A callback to be run with a PaymentIntent response from the Stripe API.
+
+ @param paymentIntent The Stripe PaymentIntent from the response. Will be nil if an error occurs. @see STPPaymentIntent
+ @param error The error returned from the response, or nil in one occurs. @see StripeError.h for possible values.


if* none* occurs

looks like similar typos elsewhere in this file

joeydong-stripe · 2018-07-09T21:13:43Z

Stripe/PublicHeaders/STPPaymentIntent.h

+@property (nonatomic, readonly) NSString *clientSecret;
+
+/**
+ The amount associated with the PaymentIntent


Our doc has a better sounding description to me: "Amount intended to be collected by this PaymentIntent."

joeydong-stripe · 2018-07-09T21:15:00Z

Stripe/PublicHeaders/STPPaymentIntent.h

+@property (nonatomic, readonly) NSNumber *amount;
+
+/**
+ If status is canceled, when the PaymentIntent was canceled.


"If status is STPPaymentIntentStatusCanceled, when the PaymentIntent was canceled."?

joeydong-stripe · 2018-07-09T21:16:47Z

Stripe/PublicHeaders/STPPaymentIntent.h

+@property (nonatomic, nullable, readonly) NSDate *created;
+
+/**
+ The currency associated with the PaymentIntent.


Our docs nicely mention it's the ISO currency code, I wonder if we mention that elsewhere we reference currency:

"Three-letter ISO currency code, in lowercase. Must be a supported currency."

https://www.iso.org/iso-4217-currency-codes.html
https://stripe.com/docs/currencies

It looks mixed. Most headers that have a currency don't provide more details, but a couple do. Similarly, we don't explain amount being the smallest currency unit in most places that have an amount.

I think it's reasonable to elide this

joeydong-stripe · 2018-07-09T21:21:56Z

Tests/Tests/STPPaymentIntentTest.m

+    XCTAssertNotNil(paymentIntent);
+    NSString *desc = paymentIntent.description;
+    XCTAssertTrue([desc containsString:NSStringFromClass([paymentIntent class])]);
+    XCTAssertGreaterThan(desc.length, 500UL, @"Custom description should be long");


copy/paste error called them `Stripe` extensions

"in one" & "in none" -> "if none"

joeydong-stripe

👍

* Delete invalid cookies * Add test

danj-stripe mentioned this pull request Jul 6, 2018

Add method to API that takes STPPaymentIntentParams and confirms them #986

Merged

danj-stripe force-pushed the danj/feature/retrieve-paymentintent branch from 29867d2 to dc9c943 Compare July 6, 2018 01:28

danj-stripe mentioned this pull request Jul 6, 2018

Add support for STPPaymentIntent to STPRedirectContext #987

Merged

Add STPPaymentIntent, and method to retrieve from the API given a cli…

1ed8e33

…entSecret Still needs implementation of `STPPaymentIntent description`, and better tests for decoding from JSON response

danj-stripe force-pushed the danj/feature/retrieve-paymentintent branch from dc9c943 to 1ed8e33 Compare July 6, 2018 02:03

danj-stripe assigned csabol-stripe Jul 6, 2018

danj-stripe added 4 commits July 6, 2018 14:05

Add test for STPPaymentIntent description, and json file with a Payme…

2328380

…ntIntent

Add tests for STPPaymentIntent STPAPIResponseDecodable conformance

96a3628

Adding a canceled_at time to the JSON, otherwise the test that the allResponseFields matches the json object fails - because we strip null values out.

Fix ConfirmationMethod case: publishable not public.

de5ce3c

joeydong-stripe reviewed Jul 9, 2018

View reviewed changes

joeydong-stripe assigned danj-stripe Jul 9, 2018

joeydong-stripe reviewed Jul 9, 2018

View reviewed changes

joeydong-stripe unassigned csabol-stripe Jul 9, 2018

danj-stripe added 3 commits July 10, 2018 11:26

Fix header comment on STPAPIClient extensions

628cb5e

copy/paste error called them `Stripe` extensions

Fix copy/pasted error across error parameter documentation

532e9be

"in one" & "in none" -> "if none"

Reword property documentation on STPPaymentIntent

6136e70

danj-stripe assigned joeydong-stripe and unassigned danj-stripe Jul 10, 2018

joeydong-stripe approved these changes Jul 10, 2018

View reviewed changes

stripe-ci added the approved label Jul 10, 2018

stripe-ci unassigned joeydong-stripe Jul 10, 2018

stripe-ci assigned danj-stripe Jul 10, 2018

danj-stripe merged commit 73a6f75 into master Jul 10, 2018

danj-stripe deleted the danj/feature/retrieve-paymentintent branch August 3, 2018 20:34

csabol-stripe pushed a commit that referenced this pull request Apr 26, 2022

[Link] Delete invalid session cookies (#985)

f6dbfa8

* Delete invalid cookies * Add test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add STPPaymentIntent, and method to retrieve from the API given a clientSecret #985

Add STPPaymentIntent, and method to retrieve from the API given a clientSecret #985

danj-stripe commented Jul 6, 2018

danj-stripe commented Jul 6, 2018

danj-stripe commented Jul 9, 2018

joeydong-stripe Jul 9, 2018

joeydong-stripe Jul 9, 2018

joeydong-stripe Jul 9, 2018

joeydong-stripe Jul 9, 2018

joeydong-stripe Jul 9, 2018 •

edited

Loading

danj-stripe Jul 10, 2018

joeydong-stripe Jul 9, 2018

joeydong-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

joeydong-stripe Jul 9, 2018

joeydong-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

joeydong-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

joeydong-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

joeydong-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

joeydong-stripe Jul 9, 2018 •

edited

Loading

joeydong-stripe Jul 9, 2018

joeydong-stripe Jul 9, 2018 •

edited

Loading

joeydong-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

joeydong-stripe Jul 9, 2018

joeydong-stripe left a comment

Add STPPaymentIntent, and method to retrieve from the API given a clientSecret #985

Add STPPaymentIntent, and method to retrieve from the API given a clientSecret #985

Conversation

danj-stripe commented Jul 6, 2018

Summary

Motivation

Testing

danj-stripe commented Jul 6, 2018

danj-stripe commented Jul 9, 2018

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

joeydong-stripe Jul 9, 2018 • edited Loading

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

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

joeydong-stripe Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeydong-stripe Jul 9, 2018 • edited Loading

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

joeydong-stripe left a comment

Choose a reason for hiding this comment

joeydong-stripe Jul 9, 2018 •

edited

Loading

joeydong-stripe Jul 9, 2018 •

edited

Loading

joeydong-stripe Jul 9, 2018 •

edited

Loading