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

danj-stripe · 2018-07-06T01:16:48Z

Summary

Adding support for /v1/payment_intents/:id/confirm

This method takes a STPPaymentIntentParams object, which may have STPSourceParams or a
Source id, among other fields.

Motivation

IOS-792

Testing

Added tests. This is part of a series of PRs, starting with #985 (which this relies on),
and this was also tested in the sample app.

…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.

Still missing `description` implementation and test for it

Includes functional test of the API method. Does not include analytics call

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

This also needs review.

csabol-stripe

overall looks good. Just a couple small comments

csabol-stripe · 2018-07-09T21:51:48Z

Stripe/PublicHeaders/STPAPIClient.h

+/**
+ Confirms the PaymentIntent object with the provided params object.
+
+ At a minimum, the params object must include the `clientSecret`.


maybe add docs as to what happens if it does not? completion called immediately with failure?

I'm not sure I want to promise/document what'll happen. This is a nonnull property, there's an assertion that it's not nil, and if they got past both of those I believe the web service call would happen and fail with an error.

csabol-stripe · 2018-07-09T21:54:07Z

Stripe/PublicHeaders/STPPaymentIntentParams.h

+
+ This should be a boolean NSNumber, so that it can be `nil`
+ */
+@property (nonatomic, strong, nullable, readwrite) NSNumber *saveSourceToCustomer;


what's the behavior if it is nil? Same as @no? If that's the case could this just be a bool, if not add docs describing behavior

It is important this is NSNumber - I started with bool because that's way friendlier, but quickly realized my mistake.

If nil, then it leaves the PaymentIntent.save_source_to_customer field unchanged.

If we forced it to be YES/NO, then the client app would have to decide what the value is, and it takes control away from their backend (or it forces the app developer to retrieve the PaymentIntent just to read this value to prevent overwriting the existing value).

This is true of every parameter that's nullable, and I don't think that's something that needs to be documented specifically on this one.

csabol-stripe · 2018-07-09T21:56:20Z

Stripe/STPAPIClient.m

+- (void)confirmPaymentIntentWithParams:(STPPaymentIntentParams *)paymentIntentParams
+                            completion:(STPPaymentIntentCompletionBlock)completion {
+    NSString *secret = paymentIntentParams.clientSecret;
+    NSCAssert(secret != nil, @"'secret' is required to confirm a PaymentIntent");


so is the intended behavior in release to just let the api request fail because of missing clientSecret instead of manually short-circuiting at this point?

This is following the same pattern used by createSourceWithParams:, createTokenWithParameters:, retrieveSourceWithId:, etc.

I think assertions may be enabled in Release builds.

stripe-ios/Stripe/BuildConfigurations/StripeiOS-Release.xcconfig

Line 10 in 42ccaaf

ENABLE_NS_ASSERTIONS = YES

@joeydong-stripe had a similar question about the retrieve method, and it might be that we want to change the way we do this in a future PR. https://github.com/stripe/stripe-ios/pull/985/files#r201145333

csabol-stripe · 2018-07-09T21:57:23Z

Stripe/STPPaymentIntentParams.m

+#import "STPPaymentIntentParams.h"
+#import "STPPaymentIntent+Private.h"
+
+@implementation STPPaymentIntentParams


I'll cast a vote to call this STPPaymentIntentParameters but happy either way

I think it's better to be consistent with the other "Params" classes for Connect Account, Legal Entity, Source, Bank Account, and Card

yeah noticed that later so that makes sense

csabol-stripe · 2018-07-09T21:58:08Z

Stripe/STPPaymentIntentParams.m

+- (instancetype)initWithClientSecret:(NSString *)clientSecret {
+    self = [super init];
+    if (self) {
+        _clientSecret = clientSecret;


this should be a copy

great catch, thanks!

csabol-stripe · 2018-07-09T22:00:14Z

Stripe/STPPaymentIntentParams.m

+
+#import "STPPaymentIntentParams.h"
+#import "STPPaymentIntent+Private.h"
+


I find it useful to include NS_ASSUME_NONNULL_BEGIN/END in the .m files as well. It makes sure your implementation matches your interface!

I think that'd be a good addition to our STYLEGUIDE.md and a nice enhancement to make.

I found very few implementation files in this project actually had it, and I didn't find any documentation about sticking it into the implementation. The stuff from Apple that I found only recommends putting it into headers.

csabol-stripe · 2018-07-09T22:03:33Z

Tests/Tests/STPPaymentIntentParamsTest.m

+- (void)testInit {
+    for (STPPaymentIntentParams *params in @[[[STPPaymentIntentParams alloc] initWithClientSecret:@"secret"],
+                                             [[STPPaymentIntentParams alloc] init],
+                                             [STPPaymentIntentParams new],


no personal preference but mixing alloc] init] and new] right next to each other reads a bit weird :P

Is this explicit to test that new actually calls alloc] init]? Seems like that's out of scope of something we need to test

The intention is to verify that all the ways that an STPPaymentIntentParams can be constructed are available, and yield an object with a non-nil clientSecret (since nil triggers an assert in confirm).

I don't think it adds a lot of value here, but it's also really cheap to include.

new sometimes behaves differently from init in this codebase (even though it does end up calling it). I was thinking of #886 in particular, where we mark STPCard init unavailable with the intent of preventing instantiation, but using new is a workaround/hole.

…definition.

danj-stripe

Thanks for the great review.

I think my answer to most of these is "that's the way the existing code does things", so my preference is to land as-is and write up any improvements as separate JIRAs to be prioritized against our other work, but let me know if you think any of them are high enough priority to include.

danj-stripe · 2018-07-10T20:19:25Z

Stripe/PublicHeaders/STPAPIClient.h

+/**
+ Confirms the PaymentIntent object with the provided params object.
+
+ At a minimum, the params object must include the `clientSecret`.


I'm not sure I want to promise/document what'll happen. This is a nonnull property, there's an assertion that it's not nil, and if they got past both of those I believe the web service call would happen and fail with an error.

danj-stripe · 2018-07-10T20:23:07Z

Stripe/PublicHeaders/STPPaymentIntentParams.h

+
+ This should be a boolean NSNumber, so that it can be `nil`
+ */
+@property (nonatomic, strong, nullable, readwrite) NSNumber *saveSourceToCustomer;


It is important this is NSNumber - I started with bool because that's way friendlier, but quickly realized my mistake.

If nil, then it leaves the PaymentIntent.save_source_to_customer field unchanged.

If we forced it to be YES/NO, then the client app would have to decide what the value is, and it takes control away from their backend (or it forces the app developer to retrieve the PaymentIntent just to read this value to prevent overwriting the existing value).

This is true of every parameter that's nullable, and I don't think that's something that needs to be documented specifically on this one.

danj-stripe · 2018-07-10T20:32:40Z

Stripe/STPAPIClient.m

+- (void)confirmPaymentIntentWithParams:(STPPaymentIntentParams *)paymentIntentParams
+                            completion:(STPPaymentIntentCompletionBlock)completion {
+    NSString *secret = paymentIntentParams.clientSecret;
+    NSCAssert(secret != nil, @"'secret' is required to confirm a PaymentIntent");


This is following the same pattern used by createSourceWithParams:, createTokenWithParameters:, retrieveSourceWithId:, etc.

I think assertions may be enabled in Release builds.

stripe-ios/Stripe/BuildConfigurations/StripeiOS-Release.xcconfig

Line 10 in 42ccaaf

ENABLE_NS_ASSERTIONS = YES

danj-stripe · 2018-07-10T20:34:33Z

Stripe/STPAPIClient.m

+- (void)confirmPaymentIntentWithParams:(STPPaymentIntentParams *)paymentIntentParams
+                            completion:(STPPaymentIntentCompletionBlock)completion {
+    NSString *secret = paymentIntentParams.clientSecret;
+    NSCAssert(secret != nil, @"'secret' is required to confirm a PaymentIntent");


@joeydong-stripe had a similar question about the retrieve method, and it might be that we want to change the way we do this in a future PR. https://github.com/stripe/stripe-ios/pull/985/files#r201145333

danj-stripe · 2018-07-10T20:40:43Z

Stripe/STPPaymentIntentParams.m

+
+#import "STPPaymentIntentParams.h"
+#import "STPPaymentIntent+Private.h"
+


I think that'd be a good addition to our STYLEGUIDE.md and a nice enhancement to make.

I found very few implementation files in this project actually had it, and I didn't find any documentation about sticking it into the implementation. The stuff from Apple that I found only recommends putting it into headers.

danj-stripe · 2018-07-10T20:42:33Z

Stripe/STPPaymentIntentParams.m

+#import "STPPaymentIntentParams.h"
+#import "STPPaymentIntent+Private.h"
+
+@implementation STPPaymentIntentParams


I think it's better to be consistent with the other "Params" classes for Connect Account, Legal Entity, Source, Bank Account, and Card

danj-stripe · 2018-07-10T20:42:57Z

Stripe/STPPaymentIntentParams.m

+- (instancetype)initWithClientSecret:(NSString *)clientSecret {
+    self = [super init];
+    if (self) {
+        _clientSecret = clientSecret;


great catch, thanks!

danj-stripe · 2018-07-10T20:51:42Z

Tests/Tests/STPPaymentIntentParamsTest.m

+- (void)testInit {
+    for (STPPaymentIntentParams *params in @[[[STPPaymentIntentParams alloc] initWithClientSecret:@"secret"],
+                                             [[STPPaymentIntentParams alloc] init],
+                                             [STPPaymentIntentParams new],


The intention is to verify that all the ways that an STPPaymentIntentParams can be constructed are available, and yield an object with a non-nil clientSecret (since nil triggers an assert in confirm).

I don't think it adds a lot of value here, but it's also really cheap to include.

new sometimes behaves differently from init in this codebase (even though it does end up calling it). I was thinking of #886 in particular, where we mark STPCard init unavailable with the intent of preventing instantiation, but using new is a workaround/hole.

…m-paymentintent Conflicts: Stripe.xcodeproj/project.pbxproj Stripe/PublicHeaders/STPAPIClient.h Stripe/PublicHeaders/STPBlocks.h Stripe/PublicHeaders/STPPaymentIntent.h Stripe/PublicHeaders/Stripe.h Stripe/STPAPIClient.m Tests/Tests/STPPaymentIntentFunctionalTest.m

…to pass manual install test

csabol-stripe · 2018-07-11T18:20:27Z

I think my answer to most of these is "that's the way the existing code does things", so my preference is to land as-is and write up any improvements as separate JIRAs to be prioritized against our other work, but let me know if you think any of them are high enough priority to include.

Sounds good to me. Land away!

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

danj-stripe force-pushed the danj/feature/confirm-paymentintent branch from 9baa1b9 to 9b7bfa9 Compare July 6, 2018 01:29

danj-stripe mentioned this pull request Jul 6, 2018

Add 3DS example VC that uses PaymentIntents to charge the user #988

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 force-pushed the danj/feature/confirm-paymentintent branch from 9b7bfa9 to 9ea11c7 Compare July 6, 2018 02:04

danj-stripe assigned csabol-stripe Jul 6, 2018

danj-stripe added 8 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

Adding STPPaymentIntentParams plus unit test

7168857

Still missing `description` implementation and test for it

Add method to API that takes STPPaymentIntentParams and confirms them

c3143cc

Includes functional test of the API method. Does not include analytics call

Implement STPPaymentIntentParams.description and test for it

215b9d1

Add analytics call to confirm method

42b009f

danj-stripe force-pushed the danj/feature/confirm-paymentintent branch from 9ea11c7 to 42b009f Compare July 6, 2018 22:45

csabol-stripe approved these changes Jul 9, 2018

View reviewed changes

csabol-stripe assigned danj-stripe and unassigned csabol-stripe Jul 9, 2018

stripe-ci added the approved label Jul 9, 2018

Use copy in STPPaymentIntentParams.init method to match property …

c45c2a2

…definition.

stripe-ci removed the approved label Jul 10, 2018

danj-stripe commented Jul 10, 2018

View reviewed changes

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

danj-stripe changed the base branch from danj/feature/retrieve-paymentintent to master July 10, 2018 22:02

danj-stripe added 2 commits July 10, 2018 15:08

Remove local variable for secret that was only used in NSCAssert …

ac2dc95

…to pass manual install test

csabol-stripe assigned danj-stripe and unassigned csabol-stripe Jul 11, 2018

csabol-stripe added the approved label Jul 11, 2018

danj-stripe merged commit a531129 into master Jul 11, 2018

danj-stripe deleted the danj/feature/confirm-paymentintent branch July 11, 2018 18:58

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

Update tint color for bank icon and x for achv2 element (#986)

b949f82

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danj-stripe commented Jul 6, 2018

danj-stripe commented Jul 9, 2018

csabol-stripe left a comment

csabol-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

csabol-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

csabol-stripe Jul 11, 2018

csabol-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

csabol-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

csabol-stripe Jul 11, 2018

csabol-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

csabol-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

csabol-stripe Jul 9, 2018

csabol-stripe Jul 9, 2018

danj-stripe Jul 10, 2018

csabol-stripe Jul 11, 2018

danj-stripe left a comment

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

danj-stripe Jul 10, 2018

csabol-stripe commented Jul 11, 2018


		#import "STPPaymentIntentParams.h"
		#import "STPPaymentIntent+Private.h"

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

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

Conversation

danj-stripe commented Jul 6, 2018

Summary

Motivation

Testing

danj-stripe commented Jul 9, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabol-stripe commented Jul 11, 2018