-
Notifications
You must be signed in to change notification settings - Fork 992
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
Add Link warmup pane for deferred intents #4272
Conversation
a11abfb
to
eee66d1
Compare
174c526
to
45baf7b
Compare
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.
Some minor questions
...inancialConnections/StripeFinancialConnections/Source/Native/Consent/ConsentDataSource.swift
Show resolved
Hide resolved
...inancialConnections/StripeFinancialConnections/Source/Native/Consent/ConsentDataSource.swift
Outdated
Show resolved
Hide resolved
StripeFinancialConnections/StripeFinancialConnections/Source/Native/NativeFlowController.swift
Outdated
Show resolved
Hide resolved
45baf7b
to
173fbb1
Compare
if dataSource.hasConsumerSession { | ||
// We already have a consumer session, so let's us this one directly | ||
delegate?.networkingLinkLoginWarmupViewControllerDidSelectContinue(self) | ||
return | ||
} |
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.
Avoiding multiple consumer session creations.
@@ -14,8 +14,11 @@ typealias NetworkingLinkLoginWarmupFooterView = (footerView: UIView?, primaryBut | |||
|
|||
protocol NetworkingLinkLoginWarmupViewControllerDelegate: AnyObject { | |||
func networkingLinkLoginWarmupViewControllerDidSelectContinue( | |||
_ viewController: NetworkingLinkLoginWarmupViewController | |||
) | |||
func networkingLinkLoginWarmupViewControllerDidFindConsumerSession( |
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.
Split this out of the didSelectContinue
method, so we can more easily call it if we already have a consumer session.
da28621
to
7d0177a
Compare
2454838
to
594dea6
Compare
7d0177a
to
ea75d91
Compare
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.
LGTM!
struct ConsentAcquiredResult { | ||
var manifest: FinancialConnectionsSessionManifest | ||
var consumerSession: ConsumerSessionData? = nil | ||
var publishableKey: String? = 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.
Assuming this is the consumer session publishable key, should we name this property something like consumerPublishableKey
?
) | ||
promise.resolve(with: result) | ||
case .failure: | ||
let result = ConsentAcquiredResult(manifest: manifest) |
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 to confirm, we're ok failing silently here?
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.
Yes. It just means that looking up the session failed, so we’d navigate to the manifest’s nextPane
.
If that’s linkLogin
(like it is for IBP today), the user would be recognized after their email is prefilled (basically today‘s experience).
## Summary <!-- Simple summary of what was changed. --> This pull request aligns the Financial Connections iOS SDK with Android and Web, where we create a consumer session before we display any verification screens. This removes a decent amount of error handling code (for very unlikely errors) and will make implementing #4272 correctly easier. ## Motivation <!-- Why are you making this change? If it's for fixing a bug, if possible, please include a code snippet or example project that demonstrates the issue. --> ## Testing <!-- How was the code tested? Be as specific as possible. --> ## Changelog <!-- Is this a notable change that affects users? If so, add a line to `CHANGELOG.md` and prefix the line with one of the following: - [Added] for new features. - [Changed] for changes in existing functionality. - [Deprecated] for soon-to-be removed features. - [Removed] for now removed features. - [Fixed] for any bug fixes. - [Security] in case of vulnerabilities. --> --------- Co-authored-by: Mat Schmid <[email protected]>
ea75d91
to
89675e4
Compare
...inancialConnections/StripeFinancialConnections/Source/Native/Consent/ConsentDataSource.swift
Show resolved
Hide resolved
Rename `publishableKey` to `consumerPublishableKey`.
89675e4
to
efe99c7
Compare
efe99c7
to
63c1fde
Compare
Summary
This pull request improves the returning-user experience for IBP consumers when the flow is opened for deferred intents. With those, we don’t have a populated
accountholderCustomerEmailAddress
, so we need to manually look for a consumer account after the user accepts on the consent pane.It also updates the warmup pane to only lookup a consumer session if there isn’t one yet.
Motivation
Testing
Changelog