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

Fix odd behavior in Stripe.retrievePossibleBrands #6977

Merged
merged 9 commits into from
Jul 19, 2023
Merged

Fix odd behavior in Stripe.retrievePossibleBrands #6977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
50349b5
Fix odd behavior in Stripe.possibleCardBrands
tillh-stripe Jul 11, 2023
548a136
Update tests
tillh-stripe Jul 11, 2023
947bfda
Reduce queries for card account ranges
tillh-stripe Jul 13, 2023
3c4bfb2
Fix test
tillh-stripe Jul 14, 2023
79eb92d
Update remote source test for new behavior
tillh-stripe Jul 14, 2023
5b12cf2
Make minor tweaks
tillh-stripe Jul 18, 2023
133af8b
Add changelog entry
tillh-stripe Jul 18, 2023
892888f
Add more tests
tillh-stripe Jul 19, 2023
d9722cb
Remove method that’s now unused
tillh-stripe Jul 19, 2023
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion payments-core-testing/src/main/java/com/stripe/android/testing/AbsFakeStripeRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<CardMetadata> {
TODO("Not yet implemented")
}

Expand Down
12 changes: 8 additions & 4 deletions payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,27 @@ 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)
}
}

override suspend fun getAccountRanges(
cardNumber: CardNumber.Unvalidated
): List<AccountRange>? {
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)
}
}

Expand Down
3 changes: 2 additions & 1 deletion payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeStore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Comment on lines +47 to +48
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updating the key here so that we can ”force-refresh”. I’m doing this because we might already be in a corrupted state for existing users.

private const val PREF_KEY_ACCOUNT_RANGES = "key_account_ranges"
}
}
35 changes: 24 additions & 11 deletions payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,27 @@ internal class RemoteCardAccountRangeSource(
cardNumber: CardNumber.Unvalidated
): List<AccountRange>? {
return cardNumber.bin?.let { bin ->
_loading.value = true

val accountRanges =
stripeRepository.getCardMetadata(bin, requestOptions)?.accountRanges.orEmpty()
cardAccountRangeStore.save(bin, accountRanges)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We no longer store any response, as these could include null responses due to network errors. Instead, the repository will decide whether to store a result.

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()
}
}

Expand All @@ -51,4 +55,13 @@ internal class RemoteCardAccountRangeSource(
paymentAnalyticsRequestFactory.createRequest(PaymentAnalyticsEvent.CardMetadataMissingRange)
)
}

private inline fun withLoading(
block: () -> Result<List<AccountRange>>,
): Result<List<AccountRange>> {
_loading.value = true
val accountRanges = block()
tillh-stripe marked this conversation as resolved.
Show resolved Hide resolved
_loading.value = false
return accountRanges
}
}
32 changes: 10 additions & 22 deletions payments-core/src/main/java/com/stripe/android/networking/StripeApiRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<CardMetadata> {
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()
}
}

/**
Expand Down Expand Up @@ -1438,14 +1434,6 @@ class StripeApiRepository @JvmOverloads internal constructor(
}
}

private suspend fun <ModelType : StripeModel> fetchStripeModel(
apiRequest: ApiRequest,
jsonParser: ModelJsonParser<ModelType>,
onResponse: () -> Unit
): ModelType? {
return jsonParser.parse(makeApiRequest(apiRequest, onResponse).responseJson())
}

private suspend fun <ModelType : StripeModel> fetchStripeModelResult(
apiRequest: ApiRequest,
jsonParser: ModelJsonParser<ModelType>,
Expand Down
2 changes: 1 addition & 1 deletion payments-core/src/main/java/com/stripe/android/networking/StripeRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ interface StripeRepository {
suspend fun getCardMetadata(
bin: Bin,
options: ApiRequest.Options
): CardMetadata?
): Result<CardMetadata>

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
suspend fun start3ds2Auth(
Expand Down
92 changes: 79 additions & 13 deletions ...ents-core/src/test/java/com/stripe/android/cards/DefaultCardAccountRangeRepositoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -198,21 +196,50 @@ internal class DefaultCardAccountRangeRepositoryTest {
}

@Test
fun `getAccountRange should not access remote source if BIN is in store`() = runTest {
val remoteSource = mock<CardAccountRangeSource>()
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
Expand Down Expand Up @@ -287,18 +314,57 @@ internal class DefaultCardAccountRangeRepositoryTest {
}

private class FakeCardAccountRangeSource(
isLoading: Boolean = false
isLoading: Boolean = false,
private val accountRanges: List<AccountRange>? = null,
) : CardAccountRangeSource {
override suspend fun getAccountRanges(
cardNumber: CardNumber.Unvalidated
): List<AccountRange>? {
return null
return accountRanges
}

override val loading: Flow<Boolean> = flowOf(isLoading)
}

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,
),
)
}
}
Loading