-
Notifications
You must be signed in to change notification settings - Fork 645
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 SDK] Integrate with backend calls for client secret #9287
[Connect SDK] Integrate with backend calls for client secret #9287
Conversation
Risky Change This is considered a risky change because it adjusts the sample app build.gradle, please review carefully. By adding the label |
5a99909
to
5d10f7f
Compare
5b9f620
to
132cadd
Compare
Diffuse output:
APK
|
601e6a3
to
c3eb44d
Compare
132cadd
to
d6fd4aa
Compare
c3eb44d
to
57baafe
Compare
d6fd4aa
to
6c7f034
Compare
57baafe
to
b024b59
Compare
# Conflicts: # dependencies.gradle # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/networking/EmbeddedComponentService.kt # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/ui/accountonboarding/AccountOnboardingExampleActivity.kt # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/ui/accountonboarding/AccountOnboardingExampleViewModel.kt # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/ui/payouts/PayoutsExampleViewModel.kt
6c7f034
to
1422da5
Compare
dependencies.gradle
Outdated
@@ -70,6 +70,7 @@ ext.versions = [ | |||
tensorflowLite : '2.11.0', | |||
tensorflowLiteSupport : '0.4.3', | |||
testParameterInjector : '1.17', | |||
timber : '5.0.1', |
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 have a Logger class in stripe-core. Would this fit your needs?
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, this looks great! My goal is to demonstrate reasonable logging behavior in our example app - I've updated the PR to remove Timber.
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 is coming along nicely!
Approving for now as I know this is in very active development and I don't want to slow momentum. However, I have medium-high FUD around the EmbeddedComponentManager
API.
|
||
@Preview(showBackground = true) | ||
@Composable | ||
fun LaunchEmbeddedComponentsScreenPreviewWithSelectedAccount() { |
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.
Make previews private here and below?
fun LaunchEmbeddedComponentsScreenPreviewWithSelectedAccount() { | |
private fun LaunchEmbeddedComponentsScreenPreviewWithSelectedAccount() { |
} | ||
|
||
object UserAgentHeader : FoldableRequestInterceptor { | ||
private fun getUserAgent(): String { |
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: could be a val
val deserializer = object : Deserializable<T> { | ||
|
||
override fun deserialize(response: Response): T { | ||
println(response.toString()) |
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.
Remove?
fun LaunchEmbeddedComponentsScreen( | ||
embeddedComponentName: String, | ||
selectedAccount: Merchant?, | ||
connectSDKAccounts: List<Merchant>, |
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.
optional: Not that performance is a concern, but if it's easy we should enable strong skipping mode to avoid recompositions with list params in composables (plus all the other situations)
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.
Good idea. If I can apply this to just the connect module, I'll do that
@OptIn(ExperimentalMaterialApi::class) | ||
@Composable | ||
fun AccountSelector( | ||
selectedAccount: Merchant? = null, |
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:
selectedAccount: Merchant? = null, | |
selectedAccount: Merchant?, |
if (sdkPublishableKey != null && accounts != null) { | ||
val embeddedComponentManager = remember(sdkPublishableKey) { | ||
EmbeddedComponentManager( | ||
activity = this@AccountOnboardingExampleActivity, |
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.
Does EmbeddedComponentManager
really need to keep an instance of Activity? Can we pass in the activity in present*
method calls instead? Currently,
- This is a memory leak foot gun for users if they don't properly cleanup the manager when the Activity is destroyed.
- There's a disconnect in having both
activity
andfetchClientSecret
as dependencies. In which architectural layer does this class belong? What if we want to cache and share the client secret between managers? - This API prevents users from making this a singleton in their apps, and introduces a lot of friction in setting up DI.
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 are great points. This API is changing after review - my follow-up PR will make this a singleton and I believe should resolve most of these concerns, but let me know if you still have FUD after the next PR
constructor( | ||
@Suppress("UNUSED_PARAMETER") activity: ComponentActivity, | ||
@Suppress("UNUSED_PARAMETER") configuration: Configuration, | ||
@Suppress("UNUSED_PARAMETER") fetchClientSecret: FetchClientSecretCallback, | ||
) : this() { | ||
throw NotImplementedError("Not yet implemented") | ||
// TODO MXMOBILE-2760 - replace with actual implementation and remove the @Suppress annotations |
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 remove the other throw
s as well replace them with toasts instead. It was unclear to me as I was testing this when a crash was intentional versus not.
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.
Good call. This should be easy to do while in development.
import com.stripe.android.connectsdk.example.networking.Merchant | ||
|
||
@Composable | ||
fun LaunchEmbeddedComponentsScreen( |
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 thought this was an Effect
based on verb-like naming. Should change it to something like:
fun LaunchEmbeddedComponentsScreen( | |
fun EmbeddedComponentsLauncherScreen( |
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.
Good idea. This may change in an upcoming PR due to API revisions anyways, and either way I like the name better!
} | ||
|
||
@Serializable | ||
data class FetchClientSecretResponse( |
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.
style: IMO we should prefer having only one public class per file. It makes finding things a lot easier.
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.
Good call. I'll refactor in a follow-up
val state: StateFlow<PayoutsExampleState> = _state.asStateFlow() | ||
|
||
init { | ||
getAccounts() |
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'll be adding retrying later?
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 my goal. It's not strictly necessary for this example app, but I think having a strong canonical example is useful
@lng thanks for the thorough feedback! This isn't live yet and there's a number of PRs queued up behind this one, so I'll address this PR in a follow-up PR later today. |
* Integrate with backend calls for client secret # Conflicts: # dependencies.gradle # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/networking/EmbeddedComponentService.kt # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/ui/accountonboarding/AccountOnboardingExampleActivity.kt # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/ui/accountonboarding/AccountOnboardingExampleViewModel.kt # stripe-connect-example/src/main/java/com/stripe/android/connectsdk/example/ui/payouts/PayoutsExampleViewModel.kt * Fix lint * add dependencies file * Move from timber to Logger * Remove unnecessary import * fix lint for deps * Fix deps * Fix colors, fix merge confligt * Fix icon lint
* Rename stripe-connect -> connect * Rename stripe-connect-example -> connect-example * Update API - fragments, Appearance, and secret callback * Move to stringRes * PR feedback from #9287 * Refactor connectsdk to just connect for package * Remove StrongSkippingMode for now (syntax is wrong) * Update example artifact to connect from connectsdk * Add placeholder Fragment UI * update deps * Remove unneeded function * Remove unneeded import * ... -> … * Remove unneeded Manifest entry * Rename stripe-connect.api to connect.api * Restrict library group * Add API * Update dependency
Summary
Adds networking to the example backend (https://stripe-connect-mobile-example-v1.glitch.me/) for getting the publishable key, accounts, and client secret for the SDK.
Motivation
Demonstrates a canonical integration of the ConnectSDK.
Testing
Screenshots
UI
demo-1729231634.mp4
Network call logging