-
Notifications
You must be signed in to change notification settings - Fork 662
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 EmbeddedActivityLauncher #9919
base: master
Are you sure you want to change the base?
Conversation
Diffuse output:
APK
|
.../src/main/java/com/stripe/android/paymentelement/embedded/DefaultEmbeddedActivityLauncher.kt
Outdated
Show resolved
Hide resolved
paymentsheet/src/main/java/com/stripe/android/paymentelement/embedded/FormActivity.kt
Outdated
Show resolved
Hide resolved
@@ -161,6 +169,10 @@ internal class DefaultEmbeddedContentHelper @AssistedInject constructor( | |||
transitionToManageScreen = { | |||
}, | |||
transitionToFormScreen = { | |||
embeddedActivityLauncher?.launchForm( |
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 will need some way to do this, but ideally we can just pass the method straight through, and the DefaultEmbeddedContentHelper doesn't need to know directly about FormContract.Args, or how to handle the result.
paymentsheet/src/main/java/com/stripe/android/paymentelement/embedded/EmbeddedContentHelper.kt
Outdated
Show resolved
Hide resolved
paymentsheet/src/main/java/com/stripe/android/paymentelement/embedded/FormActivity.kt
Outdated
Show resolved
Hide resolved
...et/src/main/java/com/stripe/android/paymentelement/embedded/SharedPaymentElementViewModel.kt
Outdated
Show resolved
Hide resolved
ce8663f
to
93d0c5a
Compare
93d0c5a
to
09981de
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.
Can you write some tests?
We should have an easy test for FormActivity
. And there's likely a lot of other tests we can write.
paymentsheet/src/main/java/com/stripe/android/paymentelement/embedded/FormActivity.kt
Outdated
Show resolved
Hide resolved
2c98bb9
to
c8c28df
Compare
paymentsheet/src/main/java/com/stripe/android/paymentelement/embedded/EmbeddedContentHelper.kt
Show resolved
Hide resolved
finish() | ||
} | ||
) { | ||
Text(args?.selectedPaymentMethodCode ?: "Whoops") |
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.
If we return above, we don't need this "whoops" text.
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.
Wrapped in a ?.let
...et/src/main/java/com/stripe/android/paymentelement/embedded/SharedPaymentElementViewModel.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/com/stripe/android/paymentelement/embedded/DefaultEmbeddedActivityLauncher.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/com/stripe/android/paymentelement/embedded/DefaultEmbeddedActivityLauncher.kt
Outdated
Show resolved
Hide resolved
paymentsheet/src/main/java/com/stripe/android/paymentelement/EmbeddedPaymentElement.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/com/stripe/android/paymentelement/embedded/DefaultEmbeddedActivityLauncher.kt
Outdated
Show resolved
Hide resolved
{ code, metadata -> | ||
formActivityLauncher.launch(FormContract.Args(code, metadata)) | ||
} | ||
private set |
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 has no effect.
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.
Removed and made override a val as well
@@ -119,6 +125,16 @@ internal class DefaultEmbeddedContentHelper @AssistedInject constructor( | |||
) | |||
} | |||
|
|||
override fun setFormLauncher( |
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.
Do we have tests for this?
...et/src/main/java/com/stripe/android/paymentelement/embedded/SharedPaymentElementViewModel.kt
Outdated
Show resolved
Hide resolved
override fun setFormLauncher( | ||
formLauncher: ((code: String, paymentMethodMetaData: PaymentMethodMetadata?) -> Unit)? | ||
) { | ||
// NO-OP |
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 have these do something, and validate it below.
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.
Added functionality to set class property. Do we want to add logic in the validation method since it's not set/clear aren't called during initialization?
.../src/main/java/com/stripe/android/paymentelement/embedded/DefaultEmbeddedActivityLauncher.kt
Show resolved
Hide resolved
finish() | ||
} | ||
) { | ||
args?.selectedPaymentMethodCode?.let { Text(it) } |
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 know it's not null here. Can we store a local val?
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.
It has to either have null check or be cast to FormContract.Args as the compiler cannot smart cast
|
||
@Before | ||
fun setUp() { | ||
MockitoAnnotations.openMocks(this) |
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 mocks don't feel like the type of tests we should write. Is it possible to write this with fakes?
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.
Created a task to refactor here
@RunWith(RobolectricTestRunner::class) | ||
internal class EmbeddedActivityLauncherTest { | ||
|
||
private lateinit var activityResultCaller: ActivityResultCaller |
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 typically write these tests with a scenario (see
Line 133 in 83ebb13
private fun testScenario( |
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.
Updated
@RunWith(RobolectricTestRunner::class) | ||
internal class FormActivityTest { | ||
|
||
private lateinit var scenario: ActivityScenario<FormActivity> |
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 you rewrite this without lateinit vars?
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 lateinit vars
@Test | ||
fun `initEmbeddedActivityLauncher and clearEmbeddedActivityLauncher successfully init and clear formLauncher`() = | ||
testScenario { | ||
val launcher: ActivityResultLauncher<FormContract.Args> = mock() |
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 you rewrite these with fakes instead of mocks?
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.
Removed mocks
lifecycleOwner.lifecycle.addObserver( | ||
object : DefaultLifecycleObserver { | ||
override fun onDestroy(owner: LifecycleOwner) { | ||
formActivityLauncher.unregister() |
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 you write a test to ensure this is happening?
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.
Added to EmbeddedActivityLauncherTest
Summary
Add EmbeddedActivityLauncher
Add FormActivity
Motivation
MOBILESDK-2940
Testing
Changelog