diff --git a/CHANGELOG.md b/CHANGELOG.md index 8be99f307bf..e4d248e5899 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## XX.XX.XX - 2023-XX-XX +### Payments +*[FIXED][6977](https://github.com/stripe/stripe-android/pull/6977) Fixed an issue where `Stripe.retrievePossibleBrands()` returned incorrect results. + ## 20.27.2 - 2023-07-18 ### PaymentSheet diff --git a/payments-core-testing/src/main/java/com/stripe/android/testing/AbsFakeStripeRepository.kt b/payments-core-testing/src/main/java/com/stripe/android/testing/AbsFakeStripeRepository.kt index 1e701bd6a23..a3ff20242f2 100644 --- a/payments-core-testing/src/main/java/com/stripe/android/testing/AbsFakeStripeRepository.kt +++ b/payments-core-testing/src/main/java/com/stripe/android/testing/AbsFakeStripeRepository.kt @@ -238,7 +238,7 @@ abstract class AbsFakeStripeRepository : StripeRepository { return Result.failure(NotImplementedError()) } - override suspend fun getCardMetadata(bin: Bin, options: ApiRequest.Options): CardMetadata { + override suspend fun getCardMetadata(bin: Bin, options: ApiRequest.Options): Result { TODO("Not yet implemented") } diff --git a/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeRepository.kt b/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeRepository.kt index 0ac9a8c7e69..3d5af097693 100644 --- a/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeRepository.kt +++ b/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeRepository.kt @@ -14,11 +14,13 @@ internal class DefaultCardAccountRangeRepository( cardNumber: CardNumber.Unvalidated ): AccountRange? { return cardNumber.bin?.let { bin -> - if (store.contains(bin)) { + val range = if (store.contains(bin)) { inMemorySource.getAccountRange(cardNumber) } else { remoteSource.getAccountRange(cardNumber) - } ?: staticSource.getAccountRange(cardNumber) + } + + range ?: staticSource.getAccountRange(cardNumber) } } @@ -26,11 +28,13 @@ internal class DefaultCardAccountRangeRepository( cardNumber: CardNumber.Unvalidated ): List? { return cardNumber.bin?.let { bin -> - if (store.contains(bin)) { + val ranges = if (store.contains(bin)) { inMemorySource.getAccountRanges(cardNumber) } else { remoteSource.getAccountRanges(cardNumber) - } ?: staticSource.getAccountRanges(cardNumber) + } + + ranges?.takeIf { it.isNotEmpty() } ?: staticSource.getAccountRanges(cardNumber) } } diff --git a/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeStore.kt b/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeStore.kt index c881b569cbb..f123b7a7176 100644 --- a/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeStore.kt +++ b/payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeStore.kt @@ -44,7 +44,8 @@ internal class DefaultCardAccountRangeStore( internal fun createPrefKey(bin: Bin): String = "$PREF_KEY_ACCOUNT_RANGES:$bin" private companion object { - private const val PREF_FILE = "InMemoryCardAccountRangeSource.Store" + private const val VERSION = 2 + private const val PREF_FILE = "InMemoryCardAccountRangeSource.Store.$VERSION" private const val PREF_KEY_ACCOUNT_RANGES = "key_account_ranges" } } diff --git a/payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt b/payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt index c31bf75e7ad..1b9365498f1 100644 --- a/payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt +++ b/payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt @@ -26,23 +26,27 @@ internal class RemoteCardAccountRangeSource( cardNumber: CardNumber.Unvalidated ): List? { return cardNumber.bin?.let { bin -> - _loading.value = true - - val accountRanges = - stripeRepository.getCardMetadata(bin, requestOptions)?.accountRanges.orEmpty() - cardAccountRangeStore.save(bin, accountRanges) + val result = withLoading { + stripeRepository.getCardMetadata( + bin = bin, + options = requestOptions, + ).map { metadata -> + metadata.accountRanges + } + } - _loading.value = false + result.onSuccess { accountRanges -> + cardAccountRangeStore.save(bin, accountRanges) - when { - accountRanges.isNotEmpty() -> { - if (cardNumber.isValidLuhn) { + if (accountRanges.isNotEmpty()) { + val hasMatch = accountRanges.any { it.binRange.matches(cardNumber) } + if (!hasMatch && cardNumber.isValidLuhn) { onCardMetadataMissingRange() } - accountRanges } - else -> null } + + result.getOrNull() } } @@ -51,4 +55,13 @@ internal class RemoteCardAccountRangeSource( paymentAnalyticsRequestFactory.createRequest(PaymentAnalyticsEvent.CardMetadataMissingRange) ) } + + private inline fun withLoading( + block: () -> Result>, + ): Result> { + _loading.value = true + val accountRanges = block() + _loading.value = false + return accountRanges + } } diff --git a/payments-core/src/main/java/com/stripe/android/networking/StripeApiRepository.kt b/payments-core/src/main/java/com/stripe/android/networking/StripeApiRepository.kt index d9b5b62b6a3..909e4eedbdc 100644 --- a/payments-core/src/main/java/com/stripe/android/networking/StripeApiRepository.kt +++ b/payments-core/src/main/java/com/stripe/android/networking/StripeApiRepository.kt @@ -860,21 +860,17 @@ class StripeApiRepository @JvmOverloads internal constructor( override suspend fun getCardMetadata( bin: Bin, options: ApiRequest.Options - ): CardMetadata? { - return runCatching { - fetchStripeModel( - apiRequestFactory.createGet( - getEdgeUrl("card-metadata"), - options.copy(stripeAccount = null), - mapOf("key" to options.apiKey, "bin_prefix" to bin.value) - ), - CardMetadataJsonParser(bin) - ) { - // no-op - } - }.onFailure { + ): Result { + return fetchStripeModelResult( + apiRequest = apiRequestFactory.createGet( + url = getEdgeUrl("card-metadata"), + options = options.copy(stripeAccount = null), + params = mapOf("key" to options.apiKey, "bin_prefix" to bin.value), + ), + jsonParser = CardMetadataJsonParser(bin), + ).onFailure { fireAnalyticsRequest(PaymentAnalyticsEvent.CardMetadataLoadFailure) - }.getOrNull() + } } /** @@ -1438,14 +1434,6 @@ class StripeApiRepository @JvmOverloads internal constructor( } } - private suspend fun fetchStripeModel( - apiRequest: ApiRequest, - jsonParser: ModelJsonParser, - onResponse: () -> Unit - ): ModelType? { - return jsonParser.parse(makeApiRequest(apiRequest, onResponse).responseJson()) - } - private suspend fun fetchStripeModelResult( apiRequest: ApiRequest, jsonParser: ModelJsonParser, diff --git a/payments-core/src/main/java/com/stripe/android/networking/StripeRepository.kt b/payments-core/src/main/java/com/stripe/android/networking/StripeRepository.kt index 80b194db9e5..9dda2bb0ef9 100644 --- a/payments-core/src/main/java/com/stripe/android/networking/StripeRepository.kt +++ b/payments-core/src/main/java/com/stripe/android/networking/StripeRepository.kt @@ -217,7 +217,7 @@ interface StripeRepository { suspend fun getCardMetadata( bin: Bin, options: ApiRequest.Options - ): CardMetadata? + ): Result @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) suspend fun start3ds2Auth( diff --git a/payments-core/src/test/java/com/stripe/android/cards/DefaultCardAccountRangeRepositoryTest.kt b/payments-core/src/test/java/com/stripe/android/cards/DefaultCardAccountRangeRepositoryTest.kt index 01944a8d4b1..566a8737f01 100644 --- a/payments-core/src/test/java/com/stripe/android/cards/DefaultCardAccountRangeRepositoryTest.kt +++ b/payments-core/src/test/java/com/stripe/android/cards/DefaultCardAccountRangeRepositoryTest.kt @@ -26,8 +26,6 @@ import org.junit.Ignore import org.junit.Rule import org.junit.runner.RunWith import org.mockito.kotlin.mock -import org.mockito.kotlin.never -import org.mockito.kotlin.verify import org.robolectric.RobolectricTestRunner import kotlin.test.Test @@ -198,21 +196,50 @@ internal class DefaultCardAccountRangeRepositoryTest { } @Test - fun `getAccountRange should not access remote source if BIN is in store`() = runTest { - val remoteSource = mock() + fun `Should return local account ranges if BIN is in store`() = runTest { + val accountRanges = FAKE_ACCOUNT_RANGES.shuffled() + + val repository = DefaultCardAccountRangeRepository( + inMemorySource = InMemoryCardAccountRangeSource(realStore), + remoteSource = FakeCardAccountRangeSource(), + staticSource = StaticCardAccountRangeSource(), + store = realStore, + ) + + val bin = requireNotNull(CardNumberFixtures.VISA.bin) + realStore.save(bin, accountRanges) + + val ranges = repository.getAccountRanges(CardNumberFixtures.VISA) + assertThat(ranges).containsExactlyElementsIn(accountRanges) + } + + @Test + fun `Should return static account ranges if remote source fails and BIN is not in store`() = runTest { + val repository = DefaultCardAccountRangeRepository( + inMemorySource = FakeCardAccountRangeSource(accountRanges = null), + remoteSource = mock(), + staticSource = StaticCardAccountRangeSource(), + store = realStore, + ) + + val ranges = repository.getAccountRanges(CardNumberFixtures.VISA) + assertThat(ranges).containsExactlyElementsIn(VISA_ACCOUNT_RANGES) + } + + @Test + fun `Should load from remote source if BIN is not in store`() = runTest { + val accountRanges = FAKE_ACCOUNT_RANGES.shuffled() + val remoteSource = FakeCardAccountRangeSource(accountRanges = accountRanges) + val repository = DefaultCardAccountRangeRepository( inMemorySource = FakeCardAccountRangeSource(), remoteSource = remoteSource, staticSource = FakeCardAccountRangeSource(), - store = realStore + store = realStore, ) - val bin = requireNotNull(CardNumberFixtures.VISA.bin) - realStore.save(bin, emptyList()) - - // should not access remote source - repository.getAccountRange(CardNumberFixtures.VISA) - verify(remoteSource, never()).getAccountRanges(CardNumberFixtures.VISA) + val ranges = repository.getAccountRanges(CardNumberFixtures.VISA) + assertThat(ranges).containsExactlyElementsIn(accountRanges) } @Test @@ -287,12 +314,13 @@ internal class DefaultCardAccountRangeRepositoryTest { } private class FakeCardAccountRangeSource( - isLoading: Boolean = false + isLoading: Boolean = false, + private val accountRanges: List? = null, ) : CardAccountRangeSource { override suspend fun getAccountRanges( cardNumber: CardNumber.Unvalidated ): List? { - return null + return accountRanges } override val loading: Flow = flowOf(isLoading) @@ -300,5 +328,43 @@ internal class DefaultCardAccountRangeRepositoryTest { private companion object { private val DEFAULT_OPTIONS = ApiRequest.Options("pk_test_vOo1umqsYxSrP5UXfOeL3ecm") + + private val VISA_ACCOUNT_RANGES = listOf( + AccountRange( + binRange = BinRange( + low = "4000000000000000", + high = "4999999999999999" + ), + panLength = 16, + brandInfo = AccountRange.BrandInfo.Visa, + ) + ) + + private val FAKE_ACCOUNT_RANGES = listOf( + AccountRange( + binRange = BinRange( + low = "4000000000000000", + high = "4999999999999999" + ), + panLength = 16, + brandInfo = AccountRange.BrandInfo.Visa, + ), + AccountRange( + binRange = BinRange( + low = "2221000000000000", + high = "2720999999999999" + ), + panLength = 16, + brandInfo = AccountRange.BrandInfo.Mastercard, + ), + AccountRange( + BinRange( + low = "5100000000000000", + high = "5599999999999999" + ), + panLength = 16, + brandInfo = AccountRange.BrandInfo.Mastercard, + ), + ) } } diff --git a/payments-core/src/test/java/com/stripe/android/cards/RemoteCardAccountRangeSourceTest.kt b/payments-core/src/test/java/com/stripe/android/cards/RemoteCardAccountRangeSourceTest.kt index d94a7b3678b..1874b207f4a 100644 --- a/payments-core/src/test/java/com/stripe/android/cards/RemoteCardAccountRangeSourceTest.kt +++ b/payments-core/src/test/java/com/stripe/android/cards/RemoteCardAccountRangeSourceTest.kt @@ -20,6 +20,7 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.robolectric.RobolectricTestRunner +import java.io.IOException import kotlin.test.Test @RunWith(RobolectricTestRunner::class) @@ -54,10 +55,6 @@ internal class RemoteCardAccountRangeSourceTest { country = "GB" ) ) - verify(cardAccountRangeStore).save( - BinFixtures.VISA, - AccountRangeFixtures.DEFAULT - ) } @Test @@ -79,12 +76,61 @@ internal class RemoteCardAccountRangeSourceTest { CardNumberFixtures.VISA ) ).isNull() - verify(cardAccountRangeStore).save( - BinFixtures.VISA, - emptyList() - ) } + @Test + fun `getAccountRange() stores server response if not empty`() = runTest { + val expectedRanges = AccountRangeFixtures.DEFAULT + + val remoteCardAccountRangeSource = RemoteCardAccountRangeSource( + stripeRepository = FakeStripeRepository(VISA_METADATA), + requestOptions = REQUEST_OPTIONS, + cardAccountRangeStore = cardAccountRangeStore, + analyticsRequestExecutor = {}, + paymentAnalyticsRequestFactory = PaymentAnalyticsRequestFactory( + ApplicationProvider.getApplicationContext(), + ApiKeyFixtures.FAKE_PUBLISHABLE_KEY + ) + ) + + remoteCardAccountRangeSource.getAccountRange(CardNumberFixtures.VISA) + verify(cardAccountRangeStore).save(BinFixtures.VISA, expectedRanges) + } + + @Test + fun `getAccountRange() does store server response even if empty`() = runTest { + val remoteCardAccountRangeSource = RemoteCardAccountRangeSource( + stripeRepository = FakeStripeRepository(EMPTY_METADATA), + requestOptions = REQUEST_OPTIONS, + cardAccountRangeStore = cardAccountRangeStore, + analyticsRequestExecutor = {}, + paymentAnalyticsRequestFactory = PaymentAnalyticsRequestFactory( + ApplicationProvider.getApplicationContext(), + ApiKeyFixtures.FAKE_PUBLISHABLE_KEY + ) + ) + + remoteCardAccountRangeSource.getAccountRange(CardNumberFixtures.VISA) + verify(cardAccountRangeStore).save(BinFixtures.VISA, emptyList()) + } + + @Test + fun `getAccountRange() does not store server response if the request failed`() = runTest { + val remoteCardAccountRangeSource = RemoteCardAccountRangeSource( + stripeRepository = FakeStripeRepository(FAILED_REQUEST), + requestOptions = REQUEST_OPTIONS, + cardAccountRangeStore = cardAccountRangeStore, + analyticsRequestExecutor = {}, + paymentAnalyticsRequestFactory = PaymentAnalyticsRequestFactory( + ApplicationProvider.getApplicationContext(), + ApiKeyFixtures.FAKE_PUBLISHABLE_KEY + ) + ) + + remoteCardAccountRangeSource.getAccountRange(CardNumberFixtures.VISA) + verify(cardAccountRangeStore, never()).save(any(), any()) + } + @Test fun `getAccountRange() when card number is less than required BIN length should return null`() = runTest { @@ -118,17 +164,19 @@ internal class RemoteCardAccountRangeSourceTest { val remoteCardAccountRangeSource = RemoteCardAccountRangeSource( FakeStripeRepository( - CardMetadata( - bin = BinFixtures.VISA, - accountRanges = listOf( - AccountRange( - binRange = BinRange( - low = "4242420000000000", - high = "4242424200000000" - ), - panLength = 16, - brandInfo = AccountRange.BrandInfo.Visa, - country = "GB" + Result.success( + CardMetadata( + bin = BinFixtures.VISA, + accountRanges = listOf( + AccountRange( + binRange = BinRange( + low = "4242420000000000", + high = "4242424200000000" + ), + panLength = 16, + brandInfo = AccountRange.BrandInfo.Visa, + country = "GB" + ) ) ) ) @@ -155,12 +203,12 @@ internal class RemoteCardAccountRangeSourceTest { } private class FakeStripeRepository( - private val cardMetadata: CardMetadata + private val result: Result ) : AbsFakeStripeRepository() { override suspend fun getCardMetadata( bin: Bin, options: ApiRequest.Options - ) = cardMetadata + ): Result = result } private companion object { @@ -168,14 +216,20 @@ internal class RemoteCardAccountRangeSourceTest { apiKey = ApiKeyFixtures.FAKE_PUBLISHABLE_KEY ) - private val EMPTY_METADATA = CardMetadata( - bin = BinFixtures.FAKE, - accountRanges = emptyList() + private val FAILED_REQUEST = Result.failure(IOException()) + + private val EMPTY_METADATA = Result.success( + CardMetadata( + bin = BinFixtures.FAKE, + accountRanges = emptyList() + ) ) - private val VISA_METADATA = CardMetadata( - bin = BinFixtures.VISA, - accountRanges = AccountRangeFixtures.DEFAULT + private val VISA_METADATA = Result.success( + CardMetadata( + bin = BinFixtures.VISA, + accountRanges = AccountRangeFixtures.DEFAULT + ) ) } } diff --git a/payments-core/src/test/java/com/stripe/android/networking/StripeApiRepositoryTest.kt b/payments-core/src/test/java/com/stripe/android/networking/StripeApiRepositoryTest.kt index cb187d685bf..c3a7489b3eb 100644 --- a/payments-core/src/test/java/com/stripe/android/networking/StripeApiRepositoryTest.kt +++ b/payments-core/src/test/java/com/stripe/android/networking/StripeApiRepositoryTest.kt @@ -1127,8 +1127,7 @@ internal class StripeApiRepositoryTest { stripeApiRepository.getCardMetadata( BinFixtures.VISA, ApiRequest.Options(ApiKeyFixtures.DEFAULT_PUBLISHABLE_KEY) - ) - requireNotNull(cardMetadata) + ).getOrThrow() assertThat(cardMetadata.bin) .isEqualTo(BinFixtures.VISA) assertThat(cardMetadata.accountRanges)