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
[FC] Unify sync API calls and add refetch strategy #8317
Conversation
Diffuse output:
APK
|
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.
Love the simplification! Only a few small comments.
) | ||
} | ||
|
||
sealed interface FetchCondition { | ||
fun check(response: SynchronizeSessionResponse): Boolean |
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.
Nit: The naming doesn’t tell what the result indicates. Can we rename this to shouldForceRefresh
or shouldLoadFromBackend
or something like that?
This also makes it easier to understand at the call site:
cachedSync.takeUnless { shouldLoadFromBackend(it) }
data object IfMissing : FetchCondition { | ||
override fun check(response: SynchronizeSessionResponse): Boolean { | ||
return false | ||
} | ||
} |
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.
Nit: Should we rename this NoFetch
or something along those lines? The case itself doesn’t have any “reload if missing” logic in it.
): SynchronizeSessionResponse = mutex.withLock { | ||
val financialConnectionsRequest = | ||
synchronizeRequest( | ||
return when (val cachedSync = cachedSynchronizeSessionResponse) { |
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.
Nit: We can make this a bit more succinct.
val cachedSync = cachedSynchronizeSessionResponse?.takeUnless(fetchCondition)
return cachedSync ?: synchronize(applicationId, clientSecret)
// TODO REMOVE BEFORE MERGING INTEGRATION BRANCH | ||
"forced_authflow_version" to "v3", |
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.
Should we remove this now? 🤔
…muvi/unify-syncs # Conflicts: # financial-connections/src/test/java/com/stripe/android/financialconnections/features/success/SuccessViewModelTest.kt
@@ -74,7 +74,7 @@ internal class SuccessViewModelTest { | |||
activeAuthSession = ApiKeyFixtures.authorizationSession(), | |||
activeInstitution = ApiKeyFixtures.institution() | |||
) | |||
whenever(getCachedAccounts()).thenReturn(accounts.data) |
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.
Not necessary in this pull request, but a good illustration of why we want fakes instead of mocks. We should get in the habit of modeling our use cases as interfaces and then injecting a fake in the test with some hard-coded data.
Summary
We've many sync calls / usecases and it's usually confusing to decide which to use. Additionally, there's situations where we want to force a refresh, even if there's a sync response cached, when certain conditions apply.
SynchronizeFinancialConnectionsSession
GetOrFetchSync(fetch = Always)
GetManifest
GetOrFetchSync().manifest
Also adds an expandable system to add re-fetch conditions. Added a
MissingActiveAuthSession
condition that will attempt a refresh when retrieving the sync object inPartnerAuth
if the auth session does not exist in the local sync response.Motivation
Testing
Screenshots
Changelog