-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Connect] Using FinancialConnections SDK in Connect #4159
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// | ||
// FinancialConnectionsPresenter.swift | ||
// StripeConnect | ||
// | ||
// Created by Mel Ludowise on 10/18/24. | ||
// | ||
|
||
@_spi(STP) import StripeCore | ||
import StripeFinancialConnections | ||
import UIKit | ||
|
||
/// Wraps `FinancialConnectionsSheet` for easy dependency injection in tests | ||
class FinancialConnectionsPresenter { | ||
@MainActor | ||
func presentForToken( | ||
apiClient: STPAPIClient, | ||
clientSecret: String, | ||
connectedAccountId: String, | ||
from presentingViewController: UIViewController | ||
) async -> FinancialConnectionsSheet.TokenResult { | ||
let financialConnectionsSheet = FinancialConnectionsSheet( | ||
financialConnectionsSessionClientSecret: clientSecret | ||
) | ||
// FC needs the connected account ID to be configured on the API Client | ||
// Make a copy before modifying so we don't unexpectedly modify the shared API client | ||
financialConnectionsSheet.apiClient = apiClient.makeCopy() | ||
financialConnectionsSheet.apiClient.stripeAccount = connectedAccountId | ||
return await withCheckedContinuation { continuation in | ||
financialConnectionsSheet.presentForToken(from: presentingViewController) { result in | ||
continuation.resume(returning: result) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
// | ||
|
||
@_spi(STP) import StripeCore | ||
import StripeFinancialConnections | ||
@_spi(STP) import StripeUICore | ||
import UIKit | ||
import WebKit | ||
|
@@ -28,6 +29,9 @@ class ConnectComponentWebViewController: ConnectWebViewController { | |
/// Manages authenticated web views | ||
private let authenticatedWebViewManager: AuthenticatedWebViewManager | ||
|
||
/// Presents the FinancialConnectionsSheet | ||
private let financialConnectionsPresenter: FinancialConnectionsPresenter | ||
|
||
private lazy var setterMessageHandler: OnSetterFunctionCalledMessageHandler = .init(analyticsClient: analyticsClient) | ||
|
||
private var didFailLoadWithError: (Error) -> Void | ||
|
@@ -49,13 +53,15 @@ class ConnectComponentWebViewController: ConnectWebViewController { | |
// Should only be overridden for tests | ||
notificationCenter: NotificationCenter = NotificationCenter.default, | ||
webLocale: Locale = Locale.autoupdatingCurrent, | ||
authenticatedWebViewManager: AuthenticatedWebViewManager = .init() | ||
authenticatedWebViewManager: AuthenticatedWebViewManager = .init(), | ||
financialConnectionsPresenter: FinancialConnectionsPresenter = .init() | ||
Comment on lines
+56
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm starting to second guess whether these should have default values or not. I'm worried that one day we will do a handoff to a new ConnectComponentWebViewController instance and not provide the existing managers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we end up having that future case, then I agree we should stop configuring default values here since it could lead to issues. But given that we don't have that case today and these are only explicitly set for test injection, I'm not too concerned about it right now. |
||
) { | ||
self.componentManager = componentManager | ||
self.notificationCenter = notificationCenter | ||
self.webLocale = webLocale | ||
self.authenticatedWebViewManager = authenticatedWebViewManager | ||
self.didFailLoadWithError = didFailLoadWithError | ||
self.financialConnectionsPresenter = financialConnectionsPresenter | ||
|
||
let config = WKWebViewConfiguration() | ||
|
||
|
@@ -66,7 +72,7 @@ class ConnectComponentWebViewController: ConnectWebViewController { | |
// embedded in the web view instead of full screen. Also works for | ||
// embedded YouTube videos. | ||
config.allowsInlineMediaPlayback = true | ||
let allowedHosts = (StripeConnectConstants.allowedHosts + [self.componentManager.baseURL.host]).compactMap({$0}) | ||
let allowedHosts = (StripeConnectConstants.allowedHosts + [self.componentManager.baseURL.host]).compactMap({ $0 }) | ||
let analyticsClient = analyticsClientFactory(.init( | ||
params: ConnectJSURLParams(component: componentType, apiClient: componentManager.apiClient, publicKeyOverride: componentManager.publicKeyOverride))) | ||
super.init( | ||
|
@@ -115,7 +121,8 @@ class ConnectComponentWebViewController: ConnectWebViewController { | |
// Should only be overridden for tests | ||
notificationCenter: NotificationCenter = NotificationCenter.default, | ||
webLocale: Locale = Locale.autoupdatingCurrent, | ||
authenticatedWebViewManager: AuthenticatedWebViewManager = .init()) { | ||
authenticatedWebViewManager: AuthenticatedWebViewManager = .init(), | ||
financialConnectionsPresenter: FinancialConnectionsPresenter = .init()) { | ||
self.init(componentManager: componentManager, | ||
componentType: componentType, | ||
loadContent: loadContent, | ||
|
@@ -124,7 +131,8 @@ class ConnectComponentWebViewController: ConnectWebViewController { | |
didFailLoadWithError: didFailLoadWithError, | ||
notificationCenter: notificationCenter, | ||
webLocale: webLocale, | ||
authenticatedWebViewManager: authenticatedWebViewManager) | ||
authenticatedWebViewManager: authenticatedWebViewManager, | ||
financialConnectionsPresenter: financialConnectionsPresenter) | ||
} | ||
|
||
required init?(coder: NSCoder) { | ||
|
@@ -262,6 +270,9 @@ private extension ConnectComponentWebViewController { | |
addMessageHandler(OpenAuthenticatedWebViewMessageHandler(analyticsClient: analyticsClient) { [weak self] payload in | ||
self?.openAuthenticatedWebView(payload) | ||
}) | ||
addMessageHandler(OpenFinancialConnectionsMessageHandler(analyticsClient: analyticsClient) { [weak self] payload in | ||
self?.openFinancialConnections(payload) | ||
}) | ||
} | ||
|
||
/// Adds NotificationCenter observers | ||
|
@@ -304,4 +315,33 @@ private extension ConnectComponentWebViewController { | |
} | ||
} | ||
} | ||
|
||
func openFinancialConnections(_ args: OpenFinancialConnectionsMessageHandler.Payload) { | ||
Task { @MainActor in | ||
let result = await financialConnectionsPresenter.presentForToken( | ||
apiClient: componentManager.apiClient, | ||
clientSecret: args.clientSecret, | ||
connectedAccountId: args.connectedAccountId, | ||
from: self | ||
) | ||
|
||
var token: String? | ||
// TODO: MXMOBILE-2491 Log these as errors instead of printing to console | ||
|
||
switch result { | ||
case .completed(result: (_, let returnedToken)): | ||
token = returnedToken?.id | ||
if returnedToken == nil { | ||
debugPrint("Error using FinancialConnections: no bank token returned") | ||
} | ||
case .failed(let error): | ||
debugPrint("Error using FinancialConnections: \(error)") | ||
case .canceled: | ||
// No-op | ||
break | ||
} | ||
|
||
sendMessage(ReturnedFromFinancialConnectionsSender(payload: .init(bankToken: token, id: args.id))) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// | ||
// OpenFinancialConnectionsMessageHandler.swift | ||
// StripeConnect | ||
// | ||
// Created by Mel Ludowise on 10/17/24. | ||
// | ||
|
||
/// Indicates to open the FinancialConnections flow | ||
class OpenFinancialConnectionsMessageHandler: ScriptMessageHandler<OpenFinancialConnectionsMessageHandler.Payload> { | ||
struct Payload: Codable, Equatable { | ||
/// The Financial Connections Session client secret used to open the FinancialConnectionsSheet | ||
let clientSecret: String | ||
/// Unique identifier (UUID) returned to the web view with the FinancialConnections | ||
/// result in `returnedFromFinancialConnections` message | ||
let id: String | ||
// The id of the Connected Account that requested the Financial Connections Session client secret | ||
let connectedAccountId: String | ||
} | ||
init(analyticsClient: ComponentAnalyticsClient, | ||
didReceiveMessage: @escaping (Payload) -> Void) { | ||
super.init(name: "openFinancialConnections", | ||
analyticsClient: analyticsClient, | ||
didReceiveMessage: didReceiveMessage) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// | ||
// ReturnedFromFinancialConnectionsSender.swift | ||
// StripeConnect | ||
// | ||
// Created by Mel Ludowise on 10/17/24. | ||
// | ||
|
||
/// Notifies that the user finished the FinancialConnections flow | ||
struct ReturnedFromFinancialConnectionsSender: MessageSender { | ||
struct Payload: Codable, Equatable { | ||
/// The linked bank account token. | ||
/// This value will be nil if the user canceled the flow or an error occurred. | ||
let bankToken: String? | ||
/// Unique identifier (UUID) originally passed from the web layer in `openFinancialConnections` | ||
let id: String | ||
} | ||
let name: String = "returnedFromFinancialConnections" | ||
let payload: Payload | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// | ||
// OpenFinancialConnectionsMessageHandlerTests.swift | ||
// StripeConnect | ||
// | ||
// Created by Mel Ludowise on 10/18/24. | ||
// | ||
|
||
@testable import StripeConnect | ||
import XCTest | ||
|
||
class OpenFinancialConnectionsMessageHandlerTests: ScriptWebTestBase { | ||
func testMessageSend() { | ||
let expectation = self.expectation(description: "Message received") | ||
webView.addMessageHandler(messageHandler: OpenFinancialConnectionsMessageHandler(analyticsClient: MockComponentAnalyticsClient(commonFields: .mock)) { payload in | ||
XCTAssertEqual(payload, .init(clientSecret: "secret_123", id: "1234", connectedAccountId: "acct_1234")) | ||
expectation.fulfill() | ||
}) | ||
|
||
webView.evaluateOpenFinancialConnectionsWebView(clientSecret: "secret_123", id: "1234", connectedAccountId: "acct_1234") | ||
|
||
waitForExpectations(timeout: TestHelpers.defaultTimeout, handler: nil) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to check to see if we have weighed the pros and cons of checked vs unchecked continuation here. If we stick with checked then this will cause a crash if resume is called more than once. It shouldn't get called more than once in this instance, but maybe we can add some extra protections here to ensure we don't call resume more than once and instead of crashing we log an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't spend too much time thinking about it in this instance since it shouldn't be possible for it to get called more than once. I don't think we have a strong opinion as to which to use the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that's ok, I don't think this should be a blocker or anything. Just wanted to call out that we have been bitten by this before. Especially within an SDK it would be ideal to not crash on a programmer error like this.