Connect Account tokenization #876

danj-stripe · 2017-12-21T23:39:58Z

Summary

Add API method for creating Connect Account tokens.

Motivation

Supporting Stripe API changes.

Testing

Unit and functional tests have been added. Manually tested with tokens created by this code to ensure I could consume them from a connect platform (although the tokens generated by the fully specific fixture in the functional test aren't consumable, they don't pass the required data validation)

danj-stripe · 2017-12-21T23:44:44Z

Stripe/PublicHeaders/STPAPIClient.h

+ */
+@interface STPAPIClient (ConnectAccounts)
+
+- (void)createTokenWithConnectAccount:(id)obj completion:(__nullable STPTokenCompletionBlock)completion;


obj needs to turn into:

account: { legal_entity: {...}, tos_shown_and_accepted: true, }

with either legal_entity or tos_shown_and_accepted, or both.

I don't know whether I want to create another ...Params class, or change this method to take two nullable parameters.

I also think it's unfortunate that Bool? is nullable NSNumber in Obj-C

Hmm, I think it'd be better to make this method take parameters rather than make a parent AccountParams class, but don't feel strongly.

Is it necessary to model tos_shown_and_accepted as a Bool?, or can we treat tos_shown_and_accepted: nil as tos_shown_and_accepted: false?

I don't think that would work. Valid values for tos_shown_and_accepted are {true, nil}. Some basic use cases:

account: { tos_shown_and_accepted: true }: minimum required to set up a new account

account: { legal_entity: { ... }, tos_shown_and_accepted: true }: setting up new account with some data about the legal entity

account: { legal_entity: { ... } }: valid for updating a pre-existing account

account: { tos_shown_and_accepted: false, ... }: rejected by the server as invalid

As I understand it, the initial purpose for this API is collecting positive affirmation that the user has seen & accepted the ToS, instead of the old world where the platform is allowed to vouch that fact as true. However, it's also just as usable for updating the legal entity info directly.

I'll think about AccountParams a little more. I like that it's easier to add new fields in the future, and the possibility of providing >1 constructors to help understand the legal_entity/tos_shown_and_accepted distinction. But it's a lot just for two optional fields

ah interesting, if that's the case then perhaps an AccountParams object with multiple constructors makes sense. thanks for the explanation!

danj-stripe · 2017-12-21T23:47:07Z

Stripe/STPLegalEntityParams.h

+};
+
+
+@interface STPLegalEntityParams : STPPersonParams


Any problems foreseen by inheriting from STPPersonParams, which conforms to STPFormEncodable?

hmm, I can't think of any

danj-stripe · 2017-12-21T23:48:15Z

Stripe/STPLegalEntityParams.h

+};
+
+
+@interface STPLegalEntityParams : STPPersonParams


Do STPVerificationParams and STPPersonParams belong in their own files? Or are they fine here?

They're only used here (for now?), and the inheritance also links STPLegalEntityParams with STPPersonParams pretty closely.

I think they're fine together!

danj-stripe · 2017-12-21T23:50:31Z

@bg-stripe WDYT? Does this look ~reasonable?

bg-stripe · 2018-01-03T16:24:13Z

this looks reasonable to me!

Adding a test to verify this works, and that this doesn't break other NSNumber instances that store 0 or 1.

danj-stripe · 2018-01-11T02:33:19Z

This includes the commit for #881, and I think it's now complete.

r? @bg-stripe

It takes a single parameter, an `STPConnectAccountParams` object. This has an optional BOOL and an optional STPLegalEntityParams object (one of fields must be provided). Still TODO: adding `description` implementations in STPLegalEntityParams.m, and testing!

This currently fails, because `legal_entity` is actually required all the time, contrary to what I thought.

These are pretty basic, but then so are the implementations of the classes. I wanted to add custom Asserts (like https://www.objc.io/issues/15-testing/xctest/#custom-assert-macros) for these *very* repetitive tests (which already exist 4x times), but I think this is okay-ish.

Last night I had moved the public headers into the right directory to fix a different failure, and rebased that into 3f2bc39. Now that bg-stripe's reviewed this PR, not going to force-push anymore.

bg-stripe

looks good, just a few nits

bg-stripe · 2018-01-11T15:53:58Z

Stripe/PublicHeaders/STPConnectAccountParams.h

+/**
+ Initialize `STPConnectAccountParams` with tosShownAndAccepted = YES
+
+ This method cannot be called with `wasAccepted == NO`, guarded by a `NSParameterAssert()`.


Ah, interesting design choice forcing the user to explicitly pass YES. I like it.

It finally clicked for me why we model this as true/nil instead of true/false :) with shownAndAccepted=false it's unclear whether the TOS was not shown or not accepted. 🤓

bg-stripe · 2018-01-11T15:54:10Z

Stripe/PublicHeaders/STPConnectAccountParams.h

+ @param wasAccepted Must be YES, but only if the user was shown & accepted the ToS
+ @param legalEntity data about the legal entity
+ */
+- (instancetype)initTosShownAndAccepted:(BOOL)wasAccepted


Should probably be initWithTos....

bg-stripe · 2018-01-11T15:59:01Z

Stripe/PublicHeaders/STPLegalEntityParams.h

+#import "STPFormEncodable.h"
+
+@class STPAddress, STPVerificationParams;
+


NS_ASSUME_NONNULL

bg-stripe · 2018-01-11T16:02:41Z

Stripe/STPAPIClient.m

+
+@implementation STPAPIClient (ConnectAccounts)
+
+- (void)createTokenWithConnectAccount:(__unused id)account completion:(__nullable STPTokenCompletionBlock)completion {


replace __unused id with `STPConnectAccountParams *``

bg-stripe · 2018-01-11T16:04:14Z

Stripe/STPAPIClient.m

+
+- (void)createTokenWithConnectAccount:(__unused id)account completion:(__nullable STPTokenCompletionBlock)completion {
+    NSMutableDictionary *params = [[STPFormEncoder dictionaryForObject:account] mutableCopy];
+    [[STPTelemetryClient sharedInstance] addTelemetryFieldsToParams:params];


I think we probably don't need to add telemetry data.

Papertrail: bg and I discussed in Slack. Android sends telemetry for all tokens. We think this is fine, but I have an open ask out to the team that owns telemetry.

bg-stripe · 2018-01-11T16:07:20Z

Stripe/STPAddress.m

-    addressDict[@"country"] = address.country;
-    params[@"address"] = [addressDict copy];
+    // Re-use STPFormEncoder
+    params[@"address"] = [STPFormEncoder dictionaryForObject:address];


bg-stripe · 2018-01-11T16:42:41Z

Stripe/STPLegalEntityParams.m

+    return @{};
+}
+
+- (void)setAdditionalAPIParameters:(__unused NSDictionary *)additionalAPIParameters {


bg-stripe · 2018-01-11T16:52:41Z

Stripe/STPConnectAccountParams.m

+//
+
+#import "STPConnectAccountParams.h"
+


NS_ASSUME_NONNULL

bg-stripe · 2018-01-11T16:52:55Z

Stripe/STPLegalEntityParams.m

+//  Copyright © 2017 Stripe, Inc. All rights reserved.
+//
+
+#import "STPLegalEntityParams.h"


NS_ASSUME_NONNULL

danj-stripe · 2018-01-11T18:22:02Z

Also, cc @ksun-stripe, who implemented this in stripe/stripe-android#492

…galEntity:`

…t:completion:`

…ormance. Faux Pas (correctly) points out adding un-prefixed methods to a Foundation class via a category is dangerous. Probably a better thing to do is either prefix all of these methods in the protocol, or add custom support for `NSDateComponents` in `+[STPFormEncoder formEncodableValueForObject:]`. For now, I think this is safe enough.

danj-stripe · 2018-01-11T23:26:13Z

@bg-stripe I've addressed your comments (thanks for all the help!), and fixed some other problems. Travis has a 1000+ build backlog and is running at half capacity. If you want to re-review now you can, or I'll assign to you again if/when the build has succeeded.

…876) * Defines iDEAL form in JSON instead of code - Adds spec for custom_dropdown - Defines iDEAL form in JSON instead of code * Fix complaint * PR feedback * Oops, fix form specs

danj-stripe commented Dec 21, 2017

View reviewed changes

danj-stripe requested a review from bg-stripe December 21, 2017 23:49

Encode NSNumber-wrapped boolean values as true/false, not 1/0

8b3dc19

Adding a test to verify this works, and that this doesn't break other NSNumber instances that store 0 or 1.

danj-stripe force-pushed the danj/feature/connect-account-tokens branch 2 times, most recently from 80aa4e0 to d59c935 Compare January 11, 2018 02:31

danj-stripe changed the title ~~[WIP] Connect Account tokenization~~ Connect Account tokenization Jan 11, 2018

danj-stripe force-pushed the danj/feature/connect-account-tokens branch from d59c935 to 139c516 Compare January 11, 2018 04:20

danj-stripe added 4 commits January 10, 2018 21:26

Add STPFormEncoder conformance to STPAddress

c9fd9ad

Add STPConnectAccountFunctionalTests

ef9cd23

This currently fails, because `legal_entity` is actually required all the time, contrary to what I thought.

danj-stripe force-pushed the danj/feature/connect-account-tokens branch from 139c516 to b93b5ac Compare January 11, 2018 05:26

Fix build - update Xcode project with location of public headers.

e8c13fe

Last night I had moved the public headers into the right directory to fix a different failure, and rebased that into 3f2bc39. Now that bg-stripe's reviewed this PR, not going to force-push anymore.

bg-stripe suggested changes Jan 11, 2018

View reviewed changes

stripe-ci assigned danj-stripe Jan 11, 2018

danj-stripe added 6 commits January 11, 2018 10:22

Add With to `STPConnectAccountParams initWithTosShownAndAccepted:le…

e3687de

…galEntity:`

Add NS_ASSUME_NONNULL_BEGIN and NS_ASSUME_NONNULL_END annotations

c5006f9

Fix type signature in implementation of `createTokenWithConnectAccoun…

bec29a3

…t:completion:`

Fix NSArray *additionalOwners property definition, strong -> copy

64503a4

Use modern obj-c notation to create NSNumber with BOOL

ee2a520

Styleguide violation, else should be on new line

533ad2a

danj-stripe assigned bg-stripe and unassigned danj-stripe Jan 12, 2018

bg-stripe approved these changes Jan 12, 2018

View reviewed changes

stripe-ci added the approved label Jan 12, 2018

stripe-ci assigned danj-stripe and unassigned bg-stripe Jan 12, 2018

danj-stripe merged commit 3da97ec into master Jan 12, 2018

danj-stripe deleted the danj/feature/connect-account-tokens branch January 12, 2018 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect Account tokenization #876

Connect Account tokenization #876

danj-stripe commented Dec 21, 2017 •

edited

danj-stripe Dec 21, 2017

bg-stripe Jan 3, 2018

danj-stripe Jan 3, 2018

bg-stripe Jan 3, 2018

danj-stripe Dec 21, 2017

bg-stripe Jan 3, 2018

danj-stripe Dec 21, 2017

bg-stripe Jan 3, 2018

danj-stripe commented Dec 21, 2017

bg-stripe commented Jan 3, 2018

danj-stripe commented Jan 11, 2018

bg-stripe left a comment

bg-stripe Jan 11, 2018

bg-stripe Jan 11, 2018

bg-stripe Jan 11, 2018

bg-stripe Jan 11, 2018

bg-stripe Jan 11, 2018

danj-stripe Jan 11, 2018

bg-stripe Jan 11, 2018

bg-stripe Jan 11, 2018

bg-stripe Jan 11, 2018 •

edited

bg-stripe Jan 11, 2018

danj-stripe commented Jan 11, 2018

danj-stripe commented Jan 11, 2018

		#import "STPFormEncodable.h"

		@class STPAddress, STPVerificationParams;


		@implementation STPAPIClient (ConnectAccounts)

		- (void)createTokenWithConnectAccount:(__unused id)account completion:(__nullable STPTokenCompletionBlock)completion {

Connect Account tokenization #876

Connect Account tokenization #876

Conversation

danj-stripe commented Dec 21, 2017 • edited

Summary

Motivation

Testing

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 commented Dec 21, 2017

bg-stripe commented Jan 3, 2018

danj-stripe commented Jan 11, 2018

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

bg-stripe Jan 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danj-stripe commented Jan 11, 2018

danj-stripe commented Jan 11, 2018

danj-stripe commented Dec 21, 2017 •

edited

bg-stripe Jan 11, 2018 •

edited