Skip to content
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.

Sign up for GitHub

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

Invoke issuing API requests on background thread #3507

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Invoke issuing API requests on background thread #3507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5706d5d
Invoke issuing API requests on background thread
mshafrir-stripe Mar 23, 2021
f2d7eb3
Respond to comments
mshafrir-stripe Mar 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 39 additions & 25 deletions stripe/src/main/java/com/stripe/android/IssuingCardPinService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,20 @@ import com.stripe.android.Stripe.Companion.appInfo
import com.stripe.android.exception.InvalidRequestException
import com.stripe.android.networking.StripeApiRepository
import com.stripe.android.networking.StripeRepository
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.CoroutineScope
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious how you found this? DO we have checks in place to make sure it doesn't happen other places?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did a quick search, it looks like apart from the synchronous calls here, there is no prod code using runBlocking

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import kotlin.coroutines.CoroutineContext

/**
* Methods for retrieval / update of a Stripe Issuing card
*/
class IssuingCardPinService @VisibleForTesting internal constructor(
keyProvider: EphemeralKeyProvider,
private val stripeRepository: StripeRepository,
private val operationIdFactory: OperationIdFactory = StripeOperationIdFactory()
private val operationIdFactory: OperationIdFactory = StripeOperationIdFactory(),
private val workContext: CoroutineContext = Dispatchers.IO
) {
private val retrievalListeners = mutableMapOf<String, IssuingCardPinRetrievalListener>()
private val updateListeners = mutableMapOf<String, IssuingCardPinUpdateListener>()
Expand Down Expand Up @@ -133,8 +138,8 @@ class IssuingCardPinService @VisibleForTesting internal constructor(
operation: EphemeralOperation.Issuing.RetrievePin,
listener: IssuingCardPinRetrievalListener
) {
runCatching {
runBlocking {
CoroutineScope(workContext).launch {
runCatching {
requireNotNull(
stripeRepository.retrieveIssuingCardPin(
operation.cardId,
Expand All @@ -145,19 +150,23 @@ class IssuingCardPinService @VisibleForTesting internal constructor(
) {
"Could not retrieve issuing card PIN."
}
}
}.fold(
onSuccess = listener::onIssuingCardPinRetrieved,
onFailure = {
onRetrievePinError(it, listener)
}
)
}.fold(
onSuccess = { pin ->
withContext(Dispatchers.Main) {
listener.onIssuingCardPinRetrieved(pin)
}
},
onFailure = {
onRetrievePinError(it, listener)
}
)
}
}

private fun onRetrievePinError(
private suspend fun onRetrievePinError(
throwable: Throwable,
listener: IssuingCardPinRetrievalListener
) {
) = withContext(Dispatchers.Main) {
when (throwable) {
is InvalidRequestException -> {
when (throwable.stripeError?.code) {
Expand Down Expand Up @@ -213,27 +222,32 @@ class IssuingCardPinService @VisibleForTesting internal constructor(
operation: EphemeralOperation.Issuing.UpdatePin,
listener: IssuingCardPinUpdateListener
) {
runCatching {
runBlocking {
CoroutineScope(workContext).launch {
runCatching {
stripeRepository.updateIssuingCardPin(
operation.cardId,
operation.newPin,
operation.verificationId,
operation.userOneTimeCode,
ephemeralKey.secret
)
}
}.fold(
onSuccess = {
listener.onIssuingCardPinUpdated()
},
onFailure = {
onUpdatePinError(it, listener)
}
)
}.fold(
onSuccess = {
withContext(Dispatchers.Main) {
listener.onIssuingCardPinUpdated()
}
},
onFailure = {
onUpdatePinError(it, listener)
}
)
}
}

private fun onUpdatePinError(throwable: Throwable, listener: IssuingCardPinUpdateListener) {
private suspend fun onUpdatePinError(
throwable: Throwable,
listener: IssuingCardPinUpdateListener
) = withContext(Dispatchers.Main) {
when (throwable) {
is InvalidRequestException -> {
when (throwable.stripeError?.code) {
Expand Down
154 changes: 67 additions & 87 deletions stripe/src/test/java/com/stripe/android/IssuingCardPinServiceTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,68 +1,58 @@
package com.stripe.android

import androidx.test.core.app.ApplicationProvider
import com.nhaarman.mockitokotlin2.argThat
import com.google.common.truth.Truth.assertThat
import com.nhaarman.mockitokotlin2.mock
import com.nhaarman.mockitokotlin2.verify
import com.nhaarman.mockitokotlin2.whenever
import com.stripe.android.networking.ApiRequest
import com.stripe.android.networking.ApiRequestExecutor
import com.stripe.android.networking.ApiRequestMatcher
import com.stripe.android.networking.StripeApiRepository
import com.stripe.android.networking.StripeRequest
import com.stripe.android.networking.StripeResponse
import com.stripe.android.exception.InvalidRequestException
import com.stripe.android.networking.AbsFakeStripeRepository
import com.stripe.android.testharness.TestEphemeralKeyProvider
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.TestCoroutineDispatcher
import kotlinx.coroutines.test.resetMain
import kotlinx.coroutines.test.setMain
import org.json.JSONObject
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import kotlin.test.AfterTest
import kotlin.test.BeforeTest
import kotlin.test.Test

/**
* Test class for [IssuingCardPinService].
*/
@ExperimentalCoroutinesApi
@RunWith(RobolectricTestRunner::class)
class IssuingCardPinServiceTest {
private val stripeApiRequestExecutor: ApiRequestExecutor = mock()
private val retrievalListener: IssuingCardPinService.IssuingCardPinRetrievalListener = mock()
private val updateListener: IssuingCardPinService.IssuingCardPinUpdateListener = mock()

private val stripeRepository = StripeApiRepository(
ApplicationProvider.getApplicationContext(),
ApiKeyFixtures.FAKE_PUBLISHABLE_KEY,
stripeApiRequestExecutor = stripeApiRequestExecutor,
analyticsRequestExecutor = {}
)
private val testDispatcher = TestCoroutineDispatcher()

private val stripeRepository = FakeStripeRepository()
private val service = IssuingCardPinService(
TestEphemeralKeyProvider().also {
it.setNextRawEphemeralKey(EPHEMERAL_KEY.toString())
},
stripeRepository,
OperationIdFactory.get()
OperationIdFactory.get(),
testDispatcher
)

@Test
fun testRetrieval() {
val response = StripeResponse(
200,
"""
{
"card": "ic_abcdef",
"pin": "1234"
}
""".trimIndent()
)
@BeforeTest
fun setup() {
Dispatchers.setMain(testDispatcher)
}

whenever(
stripeApiRequestExecutor.execute(
argThat(
ApiRequestMatcher(
StripeRequest.Method.GET,
"https://api.stripe.com/v1/issuing/cards/ic_abcdef/pin?verification%5Bone_time_code%5D=123-456&verification%5Bid%5D=iv_abcd",
ApiRequest.Options("ek_test_123")
)
)
)
).thenReturn(response)
@AfterTest
fun cleanup() {
Dispatchers.resetMain()
testDispatcher.cleanupTestCoroutines()
}

@Test
fun `retrievePin() should call onIssuingCardPinRetrieved() on listener when successful`() {
stripeRepository.retrievedPin = { PIN }

service.retrievePin(
"ic_abcdef",
Expand All @@ -72,71 +62,37 @@ class IssuingCardPinServiceTest {
)

verify(retrievalListener)
.onIssuingCardPinRetrieved("1234")
.onIssuingCardPinRetrieved(PIN)
}

@Test
fun testUpdate() {
val response = StripeResponse(
200,
"""
{
"card": "ic_abcdef",
"pin": ""
}
""".trimIndent()
)

whenever(
stripeApiRequestExecutor.execute(
argThat(
ApiRequestMatcher(
StripeRequest.Method.POST,
"https://api.stripe.com/v1/issuing/cards/ic_abcdef/pin",
ApiRequest.Options("ek_test_123")
)
)
)
).thenReturn(response)

fun `updatePin() should call onIssuingCardPinUpdated() on listener when successful`() {
service.updatePin(
"ic_abcdef",
"1234",
"5678",
"iv_abcd",
"123-456",
updateListener
)

verify(updateListener)
.onIssuingCardPinUpdated()

assertThat(stripeRepository.updatePinCalls)
.isEqualTo(1)
}

@Test
fun testRetrievalFailsWithReason() {
val response = StripeResponse(
400,
"""
{
"error": {
"code": "incorrect_code",
"message": "Verification failed",
"type": "invalid_request_error"
}
}
""".trimIndent()
)

whenever(
stripeApiRequestExecutor.execute(
argThat(
ApiRequestMatcher(
StripeRequest.Method.GET,
"https://api.stripe.com/v1/issuing/cards/ic_abcdef/pin?verification%5Bone_time_code%5D=123-456&verification%5Bid%5D=iv_abcd",
ApiRequest.Options("ek_test_123")
)
fun `retrievePin() should call onError() on listener when there is an error`() {
stripeRepository.retrievedPin = {
throw InvalidRequestException(
stripeError = StripeError(
code = "incorrect_code",
message = "Verification failed",
type = "invalid_request_error"
)
)
).thenReturn(response)
}

service.retrievePin(
"ic_abcdef",
Expand All @@ -152,7 +108,31 @@ class IssuingCardPinServiceTest {
)
}

private class FakeStripeRepository : AbsFakeStripeRepository() {
var retrievedPin: () -> String? = { null }
var updatePinCalls = 0

override suspend fun retrieveIssuingCardPin(
cardId: String,
verificationId: String,
userOneTimeCode: String,
ephemeralKeySecret: String
): String? = retrievedPin()

override suspend fun updateIssuingCardPin(
cardId: String,
newPin: String,
verificationId: String,
userOneTimeCode: String,
ephemeralKeySecret: String
) {
updatePinCalls++
}
}

private companion object {
private const val PIN = "1234"

private val EPHEMERAL_KEY = JSONObject(
"""
{
Expand Down