-
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
Support networking relink flows #4523
Conversation
a25781c
to
ab4ac56
Compare
🚨 New dead code detected in this PR: UIColor+Extensions.swift:34 warning: Property 'textBrand' is unused
UIColor+Extensions.swift:38 warning: Property 'textDisabled' is unused
UIColor+Extensions.swift:42 warning: Property 'textCritical' is unused
UIColor+Extensions.swift:50 warning: Property 'textSuccess' is unused
UIColor+Extensions.swift:70 warning: Property 'borderCritical' is unused
UIColor+Extensions.swift:90 warning: Property 'neutral0' is unused
UIColor+Extensions.swift:114 warning: Property 'neutral300' is unused
UIColor+Extensions.swift:118 warning: Property 'neutral500' is unused
UIColor+Extensions.swift:138 warning: Property 'brand100' is unused
UIColor+Extensions.swift:150 warning: Property 'critical500' is unused
UIColor+Extensions.swift:158 warning: Property 'success100' is unused
UIColor+Extensions.swift:162 warning: Property 'success500' is unused
UIColor+Extensions.swift:182 warning: Function 'dynamic(light:dark:)' is unused
DropdownFieldElement.swift:10 warning: Imported module 'StripeCore' is unused
DropdownFieldElement.swift:267 warning: Parameter 'pickerView' is unused Please remove the dead code before merging. If this is intentional, you can bypass this check by adding the label ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with |
ab4ac56
to
4c2a151
Compare
4e8dbe9
to
6e1a4ee
Compare
d076bff
to
820d1a7
Compare
94eb9fb
to
841b0fa
Compare
fcf5ba2
to
f82ae1e
Compare
- Don't return `.canceled` for networking relink sessions - Don't use custom success message in this case
f82ae1e
to
4230f28
Compare
id: repairSession.id, | ||
flow: repairSession.flow, | ||
institutionSkipAccountSelection: nil, | ||
nextPane: .success, |
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.
This aligns with Web behavior.
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.
A few small changes needed before merging, but overall look good!
let body: [String: Any] = [ | ||
"client_secret": clientSecret, | ||
"core_authorization": coreAuthorization, | ||
"return_url": "ios", |
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.
Confused by this parameter, why not pass the real return URL?
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.
That’s what we’re doing for auth session creation, too. I assume the manifest holds onto the real URL passed in the synchronize call, but will confirm.
...ions/StripeFinancialConnections/Source/API Bindings/FinancialConnectionsAsyncAPIClient.swift
Outdated
Show resolved
Hide resolved
let flow: FinancialConnectionsAuthSession.Flow | ||
let url: String | ||
let isOauth: Bool | ||
let display: FinancialConnectionsAuthSession.Display | ||
let institution: FinancialConnectionsInstitution |
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.
These fields should all be optional, according to https://stripe.sourcegraphcloud.com/stripe-internal/pay-server/-/blob/lib/bank_connections/api/resource/authorization_repair_resource.rb
...Connections/StripeFinancialConnections/Source/Native/PartnerAuth/PartnerAuthDataSource.swift
Outdated
Show resolved
Hide resolved
7d98afd
to
ad715dd
Compare
- Make repair session properties optional - Fix incorrect `maxRetries` value - Move check into `shouldRecordAuthSessionEvent()`
ad715dd
to
311c24a
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, thanks for making all the changes!
Summary
This pull request adds support for networking relink flows in Financial Connections and Instant Bank Payments.
We reuse the existing partner auth pane, but add an optional
relinkAuthorization: String?
toPartnerAuthDataSourceImplementation
. Depending on this payload, we either create an auth session or a repair session; the latter of which we then map to an auth session on the client.This pull request also includes the minor tweak to hide the secondary button in this flow.
Motivation
Testing
Changelog