-
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
Use /institution_selected
API when consent is already acquired
#4640
base: master
Are you sure you want to change the base?
Conversation
8b8670d
to
c9a80ac
Compare
|
||
import Foundation | ||
|
||
struct FinancialConnectionsSelectInstitution: Decodable { |
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.
We can't reuse the existing Synchronize
model because the visual
field is required on that model, but empty in this API request.
@@ -110,10 +111,10 @@ struct FinancialConnectionsSessionManifest: Decodable { | |||
let skipSuccessPane: Bool? | |||
let stepUpAuthenticationRequired: Bool? | |||
let successUrl: String? | |||
let theme: Theme? |
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.
All these theme
related changes are cleanups I forgot when we switched to FinancialConnectionsAppearance
. This includes no longer needing the manually write CodingKeys
.
@@ -132,6 +133,10 @@ struct FinancialConnectionsSessionManifest: Decodable { | |||
appVerificationEnabled ?? false | |||
} | |||
|
|||
var consentAcquired: Bool { | |||
!consentRequired || (consentRequired && consentAcquiredAt != 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.
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.
Can this just be !consentRequired || consentAcquiredAt != nil
?
@@ -180,6 +185,23 @@ class InstitutionPickerViewController: UIViewController { | |||
exceptForInstitution: institution | |||
) | |||
|
|||
// If consent is already acquired, create an auth session. | |||
// Otherwise, select the institution and update the manifest. | |||
if dataSource.manifest.consentAcquired { |
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.
Web equivalent logic: https://stripe.sourcegraphcloud.com/stripe-internal/pay-server/-/blob/stripe-js-v3/src/linkedAccounts/inner/components/panes/v3/InstitutionPickerPane/index.tsx?L86-96
We don't need to check for experiment assignment here like web does - we should only create the auth session if consent is already acquired.
return self.post( | ||
resource: APIEndpointInstitutionSelected, | ||
parameters: body, | ||
useConsumerPublishableKeyIfNeeded: true |
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.
We should never have a consumer publishable key available here, so this could be false
as well, but I guess it doesn’t matter.
@@ -1180,7 +1191,7 @@ enum APIEndpoint: String { | |||
.authSessionsCancel, .authSessionsRetrieve, .authSessionsOAuthResults, | |||
.authSessionsAuthorized, .authSessionsAccounts, .authSessionsSelectedAccounts, | |||
.authSessionsEvents, .networkedAccounts, .shareNetworkedAccount, .paymentDetails, | |||
.authSessionsRepair: | |||
.authSessionsRepair, .selectInstitution: |
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.
Same as above!
@@ -132,6 +133,10 @@ struct FinancialConnectionsSessionManifest: Decodable { | |||
appVerificationEnabled ?? false | |||
} | |||
|
|||
var consentAcquired: Bool { | |||
!consentRequired || (consentRequired && consentAcquiredAt != 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.
Can this just be !consentRequired || consentAcquiredAt != nil
?
Summary
As part of the institution picker first experiment, we should use the new
/institution_selected
API instead of the/auth_session
API when consent is not already acquired. This shouldn't introduce any functional changes, and mirrors the behaviour of web: https://stripe.sourcegraphcloud.com/stripe-internal/pay-server/-/blob/stripe-js-v3/src/linkedAccounts/inner/components/panes/v3/InstitutionPickerPane/index.tsx?L86-96Next, we will be adding support for the
id_consent_content
pane.Motivation
Support institution picker first experiment.
Testing
Manually tested.
Institution picker first (calling
/institution_selected
):Screen.Recording.2025-03-07.at.4.34.41.PM.mov
Existing flow (not calling the new endpoint):
Screen.Recording.2025-03-07.at.4.35.08.PM.mov
Changelog