Support native redirects #783

bdorfman-stripe · 2017-09-01T22:08:02Z

Not going to merge immediately because I think there's still some question about where in the source return data the native url for Alipay is going to be, but this is a general framework for dealing with sources that support native redirects going forward.

bg-stripe

nice work! left a few small comments

bg-stripe · 2017-09-05T19:03:18Z

Stripe/STPRedirectContext.m

@@ -19,6 +20,8 @@

 NS_ASSUME_NONNULL_BEGIN

+typedef void (^STPNativeRedirectCompletionBlock)(BOOL success);


nit: can we call this STPBoolCompletionBlock to match our other block type names?

Hmm, should i move it to STPBlocks.h ? I wasn't sure about just naming it just BoolBlock because the bool is specifically success or failure and wasn't sure if that was generic across what all bool using blocks would be.

bg-stripe · 2017-09-05T19:14:02Z

Stripe/STPRedirectContext.m

+            [application openURL:nativeUrl options:@{} completionHandler:^(BOOL success) {
+                if (!success) {
+                    STRONG(self);
+                    self->_state = STPRedirectContextStateNotStarted;


ooc, why self->_state= instead of just _state=?

Also, can't we just use the getters/setters in this method instead of ivars? I usually only use ivars in init, or if the getter/setter has side effects. Maybe we should add something to the style guide...

You have to do self->state or you get a compiler warning about implicit retain of self.

Yeah there are some style guide stuff here I guess. I didn't actually make setters for these in the .m file, I generally prefer doing it this way, but we can change (And have a style guide conversation).

oh right, forgot about that compiler warning. 👍

bg-stripe · 2017-09-05T19:27:33Z

Stripe/STPRedirectContext.m

+        }
+        else {
+            _state = STPRedirectContextStateInProgress;
+            BOOL opened = [application openURL:nativeUrl];


OOC, do you know if this behaves differently from the new openURL:options:completionHandler: method? E.g. does the returned success value here always match the success value in the async completion block?

As far as I know it's always the same, it's described the same in the docs. The main difference in the new API afaict is the options and the asyncronicity.

bg-stripe · 2017-09-05T19:30:03Z

Tests/Tests/STPRedirectContextTest.m

+
+    OCMReject([sut startSafariViewControllerRedirectFlowFromViewController:[OCMArg any]]);
+    OCMReject([sut startSafariAppRedirectFlow]);
+    OCMVerify([applicationMock openURL:[OCMArg any]


let's check the URL argument too with OCMArg isEqual:

bg-stripe · 2017-09-05T19:31:09Z

Tests/Tests/STPRedirectContextTest.m

+    OCMReject([sut startSafariViewControllerRedirectFlowFromViewController:[OCMArg any]]);
+    OCMReject([sut startSafariAppRedirectFlow]);
+    OCMVerify([applicationMock openURL:[OCMArg any]
+                               options:[OCMArg any]


nit: we can use [OCMArg isNil]

I don't think any of these should be nil?

bg-stripe · 2017-09-05T19:32:32Z

Tests/Tests/STPRedirectContextTest.m

+    id mockVC = OCMClassMock([UIViewController class]);
+    [context startRedirectFlowFromViewController:mockVC];
+
+    OCMVerify([sut startSafariViewControllerRedirectFlowFromViewController:[OCMArg any]]);


nit: we can verify this arg is equal to mockVC

joeydong-stripe

👍 Same reservations about the state instance variable access.

joeydong-stripe · 2017-09-06T20:41:46Z

Tests/Tests/STPFixtures.h

+ (STPSource *)alipaySource;
+
+/**
+ A Source object with type Alipay


"A Source object with type Alipay and native url"* 😄

joeydong-stripe · 2017-09-06T20:43:58Z

Tests/Tests/STPRedirectContextTest.m

+
+    OCMReject([sut startSafariViewControllerRedirectFlowFromViewController:[OCMArg any]]);
+    OCMReject([sut startSafariAppRedirectFlow]);
+    OCMVerify([applicationMock openURL:[OCMArg any]


joeydong-stripe · 2017-09-06T20:44:12Z

Tests/Tests/STPRedirectContextTest.m

+    OCMReject([sut startSafariViewControllerRedirectFlowFromViewController:[OCMArg any]]);
+    OCMReject([sut startSafariAppRedirectFlow]);
+    OCMVerify([applicationMock openURL:[OCMArg any]
+                               options:[OCMArg any]


joeydong-stripe · 2017-09-06T20:47:14Z

Tests/Tests/STPRedirectContextTest.m

+    OCMReject([sut startSafariAppRedirectFlow]);
+    OCMVerify([applicationMock openURL:[OCMArg any]
+                               options:[OCMArg any]
+                     completionHandler:[OCMArg any]]);


In the past, I've been able to use an [OCMArg checkWithBlock:] to trigger the completion to test the entire flow:

OCMVerify([applicationMock openURL:options:completionHandler:[OCMArg checkWithBlock:^BOOL(void (^)completion) { completion(); }]]);

bdorfman-stripe · 2017-09-07T18:56:22Z

Updated if people want to re-review.

bg-stripe

lgtm!

bg-stripe · 2017-09-07T21:05:06Z

Tests/Tests/STPRedirectContextTest.m

+    OCMStub([applicationMock sharedApplication]).andReturn(applicationMock);
+    OCMStub([applicationMock openURL:[OCMArg any]
+                             options:[OCMArg any]
+                   completionHandler:([OCMArg invokeBlockWithArgs:@YES, nil])]);


joeydong-stripe · 2017-09-07T23:29:13Z

Tests/Tests/STPRedirectContextTest.m

+    OCMStub([applicationMock sharedApplication]).andReturn(applicationMock);
+    OCMStub([applicationMock openURL:[OCMArg any]
+                             options:[OCMArg any]
+                   completionHandler:([OCMArg invokeBlockWithArgs:@YES, nil])]);


joeydong-stripe · 2017-09-07T23:29:55Z

Tests/Tests/STPRedirectContextTest.m

-    OCMVerify([applicationMock openURL:[OCMArg any]
-                               options:[OCMArg any]
-                     completionHandler:[OCMArg any]]);
+    OCMVerify([applicationMock openURL:[OCMArg isEqual:sourceURL]


I think you can just write sourceURL instead of [OCMArg isEqual:sourceURL] in the verify calls? Not completely sure.

Yeah it wasn't clear to me if they were the same. I thought maybe one way did == sourceURL and one way did isEqual:sourceURL? But I'm not sure.

bdorfman-stripe · 2017-09-12T23:21:45Z

Some minor changes if anyone wants to re-review. Otherwise this has been tested with a live charge and is ready to merge.

bg-stripe

lgtm!

bdorfman-stripe added 2 commits September 1, 2017 12:31

Support native redirects

c6cc44b

Native redirect tests

2cfa6e3

bdorfman-stripe requested review from bg-stripe and joeydong-stripe September 1, 2017 22:08

bg-stripe suggested changes Sep 5, 2017

View reviewed changes

stripe-ci assigned bdorfman-stripe Sep 5, 2017

joeydong-stripe reviewed Sep 6, 2017

View reviewed changes

Feedback from PR

9bd56b1

bg-stripe approved these changes Sep 7, 2017

View reviewed changes

stripe-ci added the approved label Sep 7, 2017

joeydong-stripe approved these changes Sep 7, 2017

View reviewed changes

Pass up extra ios alipay params

0f0d722

stripe-ci removed the approved label Sep 11, 2017

Only redirect pending sources.

5de1550

bdorfman-stripe force-pushed the bdorfman-native-redirects branch from 7f49eaf to 5de1550 Compare September 12, 2017 23:20

bg-stripe approved these changes Sep 13, 2017

View reviewed changes

stripe-ci added the approved label Sep 13, 2017

bdorfman-stripe merged commit 4163a43 into master Sep 13, 2017

bdorfman-stripe deleted the bdorfman-native-redirects branch September 13, 2017 19:10

davidme-stripe added a commit that referenced this pull request Feb 18, 2022

Update Apple Pay Docs (#783)

f425021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support native redirects #783

Support native redirects #783

bdorfman-stripe commented Sep 1, 2017

bg-stripe left a comment

bg-stripe Sep 5, 2017

bdorfman-stripe Sep 7, 2017

bg-stripe Sep 5, 2017

bdorfman-stripe Sep 5, 2017

bg-stripe Sep 5, 2017

bg-stripe Sep 5, 2017

bdorfman-stripe Sep 5, 2017

bg-stripe Sep 5, 2017

joeydong-stripe Sep 6, 2017

bg-stripe Sep 5, 2017

joeydong-stripe Sep 6, 2017

bdorfman-stripe Sep 7, 2017

bg-stripe Sep 5, 2017

joeydong-stripe left a comment

joeydong-stripe Sep 6, 2017

joeydong-stripe Sep 6, 2017

joeydong-stripe Sep 6, 2017

joeydong-stripe Sep 6, 2017

bdorfman-stripe commented Sep 7, 2017

bg-stripe left a comment

bg-stripe Sep 7, 2017

joeydong-stripe Sep 7, 2017

joeydong-stripe Sep 7, 2017

joeydong-stripe Sep 7, 2017

bdorfman-stripe Sep 7, 2017

bdorfman-stripe commented Sep 12, 2017

bg-stripe left a comment

		@@ -19,6 +20,8 @@

		NS_ASSUME_NONNULL_BEGIN

		typedef void (^STPNativeRedirectCompletionBlock)(BOOL success);

Support native redirects #783

Support native redirects #783

Conversation

bdorfman-stripe commented Sep 1, 2017

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

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

bdorfman-stripe commented Sep 7, 2017

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

bdorfman-stripe commented Sep 12, 2017

bg-stripe left a comment

Choose a reason for hiding this comment